-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support of sharing message in multiple checkers. Fix DeprecatedChecker example #6693
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
Conversation
Pull Request Test Coverage Report for Build 2493613951
π - Coveralls |
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.
Would it be possible to add a short example in the developer guide, please ? I think this one checker is very likely to be customized. (Maybe we could provide a way to just configure what is deprecated or not in the configuration but that's probably for another MR)
I can. Should be done in this MR or should I create new one? Moreover I think, we should include in our tests execution of the examples to avoid having them outdated and not running... |
Either works :) Related to your last point about checking the exemples of the doc we'd need some kind of structure like for bad.py / good.py, but this is a really neat idea ! |
pylint/checkers/deprecated.py
Outdated
|
||
msgs: dict[str, MessageDefinitionTuple] = { | ||
DEPRECATED_MSGS: dict[str, MessageDefinitionTuple] = { |
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.
Shouldn't this be called:
DEPRECATED_MSGS: dict[str, MessageDefinitionTuple] = { | |
DEPRECATED_MSGS_STDLIB: dict[str, MessageDefinitionTuple] = { |
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.
This is a question how DeprecatedMixin is supposed to be used:
- should external users reuse DeprecatedMixin?
- if yes should they use MSGS which are used also in STDLIB checker (W15XX) and import checker (W0402)? If W15XX and W0402 are reused also for external users the renaming is confusing then because it is not connected to STDLIB only.
- if no we should somehow parametrize the mixin allowing users to specify which message should be raised
- if external users should not reuse DeprecatedMixin we can do the renaming.
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.
should external users reuse DeprecatedMixin?
I'm wondering why we do not have a configuration for deprecations checks ? Isn't it possible to have user modifiable deprecated_functions
, deprecated_classes
list options ?
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'm wondering why we do not have a configuration for deprecations checks ? Isn't it possible to have user modifiable deprecated_functions, deprecated_classes list options ?
We can provide to the user these options. Should it go with this PR?
In any case I think some users will need subclassing DeprecatedMixin
. By subclassing mixin user can check the deprecated function also based on version - e.g. django plugin can check deprecated django functions/classes based on django version. This cannot be done by simple list of functions/modules in configuration file.
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.
We can provide to the user these options. Should it go with this PR?
Let's first handle this PR.
By subclassing mixin user can check the deprecated function also based on version - e.g. django plugin can check deprecated django functions/classes based on django version.
Ha right, it make sense.
@matusvalo What about moving these messages to (for example) Edit: That also allows plugin writers to use our CI check by creating any new deprecation messages with |
@DanielNoord unfortunately I don't understand. :-( What is CI? |
Continuous Integration. The checks we run with Github Actions. The reason for the weird dictionary creation is because we have a check that makes sure that all messages in a checker have the same first two digits. This is what is causing issues for this |
for more information, see https://pre-commit.ci
I understand now. But I have a two questions:
Hence, for now I did most safest change:
|
Hm CI is failing due inconsistent Messages. Now I see why we were talking about CI... :-/ |
This comment has been minimized.
This comment has been minimized.
Let's do this with the |
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.
This is exactly what I meant! Thanks @matusvalo!
Only one issue remains:
The creation of the messages documentation. As you can see from the RTD
output we're not creating two pages for every message in the MixIn
as they are found on two checkers. Do you think you'll be able to find a solution for this or do you need some pointers? I don't have a good idea yet, but since this is based on our own Sphinx
extension it shouldn't be too hard!
@DanielNoord @Pierre-Sassoulas please have a look to PR:
|
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.
Some early feedback!
Thanks for all the work you put in to this @matusvalo!
@@ -7,7 +7,7 @@ | |||
import os |
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.
This all seems to work.
The only thing I would add is to add all checkers that can emit a message to the page. So for these messages both stdlib
and imports
. But that can also be done in a follow-up.
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.
The only thing I would add is to add all checkers that can emit a message to the page
If I understand your comment correctly, this is already implemented. See _write_single_shared_message_page
and _write_single_message_page
functions.
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.
@matusvalo Did you decide to do a follow-up? If so, could you open the issue?
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.
The only thing I would add is to add all checkers that can emit a message to the page. So for these messages both stdlib and imports. But that can also be done in a follow-up.
@DanielNoord not sure if I understand. Currently, all checkers emitting the message are showed. Currently, we don't have a Message that is used by multiple checkers, hence only one checker is showed for deprecated-*
messages:
But the logic is universal, suppose we add to the Imports
checker another message used by stdlib
checker:
When we generate documentation with this code change, following page is showed:
Co-authored-by: DaniΓ«l van Noord <[email protected]>
Co-authored-by: DaniΓ«l van Noord <[email protected]>
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.
LGTM, amazing work ! It will be useful to make the checker more modular. I wonder if you could create a small explanation on how to do that in the contributor doc on checkers ?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I saw that extra options does not have documentation. So I decided to create separate PR adding all of them: #6947 . Marked as draft because it documents also |
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.
Final nitpicks. Thanks @matusvalo π
@@ -7,7 +7,7 @@ | |||
import os |
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.
@matusvalo Did you decide to do a follow-up? If so, could you open the issue?
Co-authored-by: DaniΓ«l van Noord <[email protected]>
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.
https://github.com/PyCQA/pylint/pull/6693/files#r896124542
https://github.com/PyCQA/pylint/pull/6693/files#r896129061
@matusvalo I think you resolved these without answering the questions in them. Or am I overseeing your answers somewhere?
pylint/checkers/deprecated.py
Outdated
{"old_names": [("W1513", "old-deprecated-decorator")], "shared": True}, | ||
), | ||
} | ||
|
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 splitted the messages because when documentation is generated, the messages attached to the checkers are discovered. I wanted to avoid to listing messages not emitted by checker in the documentation (e.g. ImportsChecker
is using only deprecated-module
)
): | ||
shared_messages_list = list(shared_messages) | ||
if len(shared_messages_list) > 1: | ||
_write_single_shared_message_page(category_dir, shared_messages_list) |
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.
_write_single_shared_message_page
is used when we have multiple checkers using the same message. Currently, there is no such message, but I added it to cover all cases.
@@ -7,7 +7,7 @@ | |||
import os |
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.
The only thing I would add is to add all checkers that can emit a message to the page
If I understand your comment correctly, this is already implemented. See _write_single_shared_message_page
and _write_single_message_page
functions.
@@ -7,7 +7,7 @@ | |||
import os |
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.
The only thing I would add is to add all checkers that can emit a message to the page. So for these messages both stdlib and imports. But that can also be done in a follow-up.
@DanielNoord not sure if I understand. Currently, all checkers emitting the message are showed. Currently, we don't have a Message that is used by multiple checkers, hence only one checker is showed for deprecated-*
messages:
But the logic is universal, suppose we add to the Imports
checker another message used by stdlib
checker:
When we generate documentation with this code change, following page is showed:
@DanielNoord My bad. I had pending review, for days and I missed that. I left you multiple messages waiting for publishing π€¦ |
Well it seems I was mistaken as well. The only thing I was worried about was #6693 (comment) and you already fixed that before I even mentioned it. Thanks for all the work here @matusvalo, LVGTM π |
Thank you @DanielNoord @Pierre-Sassoulas for your review! |
Type of Changes
Description
This PR move message definitions from
msgs
attribute of DeprecatecMixin to separate class attributes.msgs
attribute cannot be used directly anyway because it mixes two different message classes:W15XX
andW04XX
.Closes #6656