-
-
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
[configuration] Create a data structure to store breaking changes #9088
[configuration] Create a data structure to store breaking changes #9088
Conversation
pylint/config/_breaking_changes.py
Outdated
[Condition.MESSAGE_IS_NOT_DISABLED, Condition.EXTENSION_IS_NOT_LOADED], | ||
[[Solution.ADD_EXTENSION], [Solution.DISABLE_MESSAGE_IMPLICITLY]], |
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.
For something like this, it's hard to know if the upgrade was done the second time the script is run. (Maybe it's disabled implicitly voluntarily).
[Condition.MESSAGE_IS_NOT_DISABLED, Condition.EXTENSION_IS_NOT_LOADED], | |
[[Solution.ADD_EXTENSION], [Solution.DISABLE_MESSAGE_IMPLICITLY]], | |
[Condition.MESSAGE_IS_NOT_DISABLED, Condition.EXTENSION_IS_NOT_LOADED], | |
[[Solution.ADD_EXTENSION], [Solution.DISABLE_MESSAGE_EXPLICITLY]], |
We might want another way to tell if the pylintrc need upgrading, like maybe a upgraded-to=3.0.0
option and keeping track of the target version in the data structure (i.e a dict[str, list[BreakingChangeWithSolution]]
``{"2.15.0": [no-self-use], "3.0.0": compare-to-zero compare-to-empty-string} and if upgraded-to=2.17.0 we do not check no-self-use anymore).
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.
Like this a lot, just some Qs.
MESSAGE_IS_ENABLED = "{symbol} ({msgid}) is enabled" | ||
MESSAGE_IS_NOT_ENABLED = "{symbol} ({msgid}) is not enabled" | ||
MESSAGE_IS_DISABLED = "{symbol} ({msgid}) is disabled" | ||
MESSAGE_IS_NOT_DISABLED = "{symbol} ({msgid}) is not disabled" |
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.
Wondering how these will work with ALL? Hopefully they won't, I guess. If there are low-quality messages we're moving to extensions, it would be a shame for people who have ALL enabled to then start explicitly enabling them.
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 did not think of all, but we definitely need to add data in the conf to be able to handle this case (#9088 (comment))
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.
If we move something to extension (like no-self-use) I was thinking the condition / solutions would be:
[Condition.MESSAGE_IS_ENABLED, Condition.EXTENSION_IS_NOT_LOADED],
[[Solution.ADD_EXTENSION], [Solution.DISABLE_MESSAGE_IMPLICITLY]],
So would all
would be activated if the condition is MESSAGE_IS_ENABLED
? Can we even see the difference between enable=all or an explicit enable=no-self-use ? In any case if someone launch all
following the upgrade we need to be able to not warn and we need an info that the pylintrc was upgraded to a certain version already.
pylint/config/_breaking_changes.py
Outdated
msgid="R6301", symbol="no-self-use", extension="pylint.extensions.no_self_use" | ||
) | ||
COMPARE_TO_ZERO = Information( | ||
msgid="C2001", symbol="compare-to-zero", extension="pylint.extensions.comparetozero" |
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.
What happens when the message ids change? (as we do somewhat frequently)
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.
Hmm... We're going to have to check with the message store (we also need the message store if we want to upgrade the message names automatically later). We have all the msgid/symbol correspondence in the message store so there's a little duplication. But not the "old extensions" infos. I wonder if we should just use msgid_or_symbol
and add the old_extensions
info in the message dict from the checker...
msgid="C2001", symbol="compare-to-zero", extension="pylint.extensions.comparetozero" | |
msgid_or_symbol="C2001", extension="pylint.extensions.comparetozero" |
msgid="C2001", symbol="compare-to-zero", extension="pylint.extensions.comparetozero" | |
msgid_or_symbol=""compare-to-zero" | |
#In the implicit_booleaness_checker .MSGS | |
... | |
old_extensions = ["pylint.extensions.comparetozero"] | |
... |
This comment has been minimized.
This comment has been minimized.
In order to be able to automate the fix later in pylint-dev#5462
899a644
to
2c75c33
Compare
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.
Looks good after Jacob's concerns have been adressed. It is a bit hard to review this without seeing it being fully used, but splitting it off in smaller PRs is indeed nice!
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit 4ca0379 |
@jacobtylerwalls what do you think of the current state of the datastructure ? Should we merge as is ? I'm planning on adding what's necessary to be able to do breaking change in option values like #8411 next. It feels like we'd need to define a function specific for each in this case ("option for overgeneral exception does not contain a dot" ) I could also try to make something more generic in this MR directly. |
Type of Changes
Description
First step for #5462 . I suppose considering the timeline early reviews are important π