-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more cases that emit bad-plugin-value #7284
Add more cases that emit bad-plugin-value #7284
Conversation
Pull Request Test Coverage Report for Build 3006894605
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
This PR now catches case 3, and there is a test for it. Will mention progress update in #7264. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Just chiming in with no time to review but it seems some cases are fixed, maybe we should merge the fixed cases right now and handle the following one later ? I'm loving the detailed specifications in markdown, that's very nice. |
Hi @Pierre-Sassoulas, it's been a busy week so I'm just coming back to this now. I don't mind doing more, smaller PRs, or anything really. Whatever approach suits your needs best. I'll probably do a bit more work on this today, if only just to confirm that case 6 is also covered by what I've done already (by adding a test). |
Case 6 covered! |
But it does reduce my email spam 😄 |
This comment has been minimized.
This comment has been minimized.
@DanielNoord could you resolve the conflict, please ? I'd be tempted to keep |
I'm on mobile, but you can change to |
Thank you for the quick feedback :) |
This comment has been minimized.
This comment has been minimized.
Hello! I'll add a towncrier fragment for just these 2 fixed cases now then, if you'd like merge this code before too much else happens on main. |
This comment has been minimized.
This comment has been minimized.
@daogilvie I'd be fine with fixing these two cases and doing the others in another PR. Could you take a look at the failing tests? Probably something to do with Windows file paths.. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The remaining failure is an unrelated documentation issue.
I don't think that requires fixing on this branch? I'll mark the PR as non-draft. |
Yeap, don't mind the documentation check failing :) |
@daogilvie Would you mind rebasing on You can ping me for a review afterwards 😄 |
Another submission for you @DanielNoord. Thank you again for your patience! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, some superficial changes to wording and docstrings.
@Pierre-Sassoulas I know we want to release, but should we merge this in 2.15.1
as well?
Thanks again @DanielNoord, your patience is appreciated. Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one final commit with some last comments, just to make things more explicit.
Thanks a lot @daogilvie. This is wonderful! Would you be able to write an update in the linked issue so that we know were we are at now that this is getting merged? And are you also planning on tackling those last issue/cases that remain?
@Pierre-Sassoulas I put this on the milestone for tonight's release. It's a nice bug fix and I think it can be included as well 😄
Happy to help! Thank you for your calm and patient guidance 😄 I'll add something to #7264 shortly |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work on this issue @daogilvie, we'll release it in 2.15.2 tonight :)
52f10ef
* Add x-failing test for issue 7264 case 3 This is the case where a plugin can be imported only after the init-hook is run, and the init hook is specified in a pylintrc file We would expect the module to not load in any case, and cause the emission of a bad-plugin-value message. * _dynamic_plugins is a dict not a set This is in preparation for caching the loaded modules, and for storing other information that will help in identifying times loading is dependent on init-hook magic. * Use cached module objects for load_configuration This fixes case 3 in pylint-dev#7264, and is technically a breaking change, in that it now emits a message for what would have previously been a silent failure. * Interim review comment fixes 1. Remove the xfail that snuck back in after the rebases. 2. Now that fake_home yields a str, update the type hint. * Add test for bad plugin with duplicated load This is case 6 in issue pylint-dev#7264, and represents the other silent failure that is being made non-silent by this change. * Apply review feedback - Add an ordering test for CLI arguments - Add clarifying comments on affected functions - Tidy up test assertions to be more pythonic Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 52f10ef |
* Add x-failing test for issue 7264 case 3 This is the case where a plugin can be imported only after the init-hook is run, and the init hook is specified in a pylintrc file We would expect the module to not load in any case, and cause the emission of a bad-plugin-value message. * _dynamic_plugins is a dict not a set This is in preparation for caching the loaded modules, and for storing other information that will help in identifying times loading is dependent on init-hook magic. * Use cached module objects for load_configuration This fixes case 3 in #7264, and is technically a breaking change, in that it now emits a message for what would have previously been a silent failure. * Interim review comment fixes 1. Remove the xfail that snuck back in after the rebases. 2. Now that fake_home yields a str, update the type hint. * Add test for bad plugin with duplicated load This is case 6 in issue #7264, and represents the other silent failure that is being made non-silent by this change. * Apply review feedback - Add an ordering test for CLI arguments - Add clarifying comments on affected functions - Tidy up test assertions to be more pythonic Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Type of Changes
Description
As per #7264, this PR will make the changes covered the lines marked with ❗ in the following table. The table is about the relationship between the
load-plugins
andinit-hook
arguments/configuration options.Refs #7264