Distinguish between genuine typos and old configuration that could be cleaned in configuration warning #6824

Pierre-Sassoulas · 2022-06-03T13:50:39Z

Type of Changes

	Type
✓	🔨 Refactoring
✓	📜 Docs

Description

Deleted messages that do not exists anymore in pylint now raise useless-option-value instead of bad-option-value
which permit to distinguish between genuine typoes and old configuration that could be cleaned.

Refs #6794, #6514 ... and 2 or 3 other issues opened recently. There's also question on stackoverflow about this.

For example:
https://stackoverflow.com/questions/72486567/
https://stackoverflow.com/questions/72478704/

Pierre-Sassoulas · 2022-06-03T13:53:21Z

pylint/constants.py

+    ],
+    "https://pylint.pycqa.org/en/latest/whatsnew/1/1.4.html#what-s-new-in-pylint-1-4-3": [
+        DeletedMessage("R0921", "abstract-class-not-used"),
+        DeletedMessage("R0922", "abstract-class-little-used"),


This was not documented before, I had to dig a little.

DanielNoord

Well done on finding a nice fix for this. This does indeed seem like a good middle-ground.

And well done on finding those old messages.

Some earlier comments before I do a full review/

DanielNoord · 2022-06-03T13:58:37Z

pylint/lint/message_state_handler.py

@@ -411,9 +412,14 @@ def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
                        try:
                            meth(msgid, "module", l_start)
                        except exceptions.UnknownMessageError:


I just wonder, there are more place where we catch this exception. Should we also add this special handling there?

Well, something really elegant would be to reraise a DeletedMessageError with the justification...

Or pass the linter to the Error and add the message from the error itself?

DanielNoord · 2022-06-03T13:58:54Z

pylint/message/message_definition_store.py

+                    return explanation
+        return None
+
+    def kwargs_for_bad_option_value_add_message(


Let's make both of these private while we haven't documented our public API specifically.

We're using it in PyLinter ourself so it's complicated to make them private. I'm also ok with poeple using the is_deleted_msgid_or_symbol as a public API. What about making kwargs_for_bad_option_value_add_message protected ?

I think private means private to the package owner. I have used underscores to denote stuff as private but then used that in other places too. As long as it is all within the same package you can't really change stuff without immediately noticing something breaks.

is_deleted_msgid_or_symbol seems like a good utility method but I just worry that down the line we might want to change stuff to it or use a completely different method of raising these messages. After the issues we had with stuff unintentionally breaking in 2.12 and 2.13 I'm just like: as long as nobody asks for it, nobody needs it and we should just keep it to ourselves to make it easier to change stuff later on.

DanielNoord · 2022-06-03T13:59:09Z

tests/config/functional/ini/pylintrc_with_missing_comma.2.out

@@ -1,3 +1,3 @@
 ************* Module {abspath}
-{relpath}:1:0: E0012: Bad option value for --disable. Don't recognize message logging-not-lazylogging-format-interpolation. (bad-option-value)
-{relpath}:1:0: E0012: Bad option value for --enable. Don't recognize message locally-disabledsuppressed-message. (bad-option-value)
+{relpath}:1:0: E0012: Bad option value for '--disable'. Don't recognize message 'logging-not-lazylogging-format-interpolation'. (bad-option-value)


We should probably join the messages with a comma.

@Pierre-Sassoulas My bad, I meant the message that we don't recognise 😅

But the comma you added now is also a good changed imo 👍

Ho 😄 well this is the content of the test, if it was properly joined with a comma there would not be a bad-option-value raised. It's admittedly a little too close to what happen in real life ezczeczasxaz would have the same effect.

Oops, I was looking at another test just above it and thought they were connected. This is indeed correct.

coveralls · 2022-06-03T14:02:10Z

Pull Request Test Coverage Report for Build 2446419315

10 of 10 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 95.531%

Totals
Change from base Build 2446340211:	0.001%
Covered Lines:	16331
Relevant Lines:	17095

💛 - Coveralls

Pierre-Sassoulas · 2022-06-03T14:53:06Z

The primer actually find something interesting in astroid, we can't use parenthesis to justify a pragma. We had a discussion about this but I think we should just fix this in 2.14.1 and do better later.

DanielNoord · 2022-06-03T15:50:55Z

The primer actually find something interesting in astroid, we can't use parenthesis to justify a pragma. We had a discussion about this but I think we should just fix this in 2.14.1 and do better later.

Should be fixed with an additional #. Other tools require this as well, so I don't think it's too big of an issue.

I believe mypy doesn't even recognise its disables if they come after an pylint disable. They want priority 😄

.pyenchant_pylint_custom_dict.txt

doc/whatsnew/2/2.14/full.rst

Pierre-Sassoulas · 2022-06-05T06:56:14Z

Sorry to make changes after your review @jacobtylerwalls, but I figured:

bad-option-value is not an "error", in the sense that it's not going to break the code, it's only a potential problem in pylint. So it make sense to make it a warning and to not be warned for this in --error-only mode (only problem could be if a user think he enabled a message and it's not enabled, but it must have been disabled previously so I think it's unlikely to happen).
We were adding a new message in a patch version, we need to use old_names so the previous disable still works. Otherwise people that already got bothered by 2.14.0 and disabled bad-option-value will still get useless-option-value... that can be annoying.

So I'm going to add a new commit using old-names this time.

DanielNoord · 2022-06-05T07:42:35Z

@Pierre-Sassoulas I'd advise against making this an I warning. I think W is better. Information warnings are turned off by default and need to be turned on individually. So nobody will likely every find this message.

doc/data/messages/u/useless-option-value/bad.py

DanielNoord · 2022-06-05T09:35:18Z

Sure, but isn't it exactly what we're doing now ? useless-option-value is not a new message, it's the new name of bad-option-value (with its sister message unknown-option-value) ? I agree we should not reuse number in new messages for precisely this scenario.

Yeah, one of them should keep 0012, but the other should get its own number. We don't really have a concept of "sister messages" and I don't think we should start creating those.

Pierre-Sassoulas · 2022-06-05T09:39:39Z

We don't really have a concept of "sister messages"

In fact the only other messages used multiple time as old_name is missing-docstring but we did not change the message type of the children so we did C0111 => [C0012 C0013 C0014]

DanielNoord · 2022-06-05T09:43:33Z

We don't really have a concept of "sister messages"

In fact the only other messages used multiple time as old_name is missing-docstring but we did not change the message type of the children so we did C0111 => [C0012 C0013 C0014]

I was more referring to messages that are related but are on different levels. To take docstring as an example. Let's say we find module docstrings more important we could have C0012 for missing-function-docstring and E0012 for missing-module-docstring.
Coupling messages across different levels like that might only create issues down the line I think. We're changing the symbols of these messages anyway and we're not running out of IDs so I really don't see why would want to couple them like that.
It is also confusing for users as we would have C0012, R0012 and W0012 which are all different messages but with almost the same id.

Pierre-Sassoulas · 2022-06-05T12:24:16Z

Sorry for rebasing but I think the commits need to be separated. I renamed R0012 to R0022 (first number not used elsewhere in main checker).

DanielNoord

Just some initial review. If you're able to perhaps split some changes from this PR. There is a lot going on, with some of it just being general refactoring. Makes it quite hard to review this.

After all the opened issues I'd like to get this right once and for all.

doc/data/messages/b/bad-option-value/bad.py

DanielNoord · 2022-06-05T20:57:48Z

doc/test_messages_documentation.py

@@ -71,7 +73,6 @@ def get_functional_test_files_from_directory(input_dir: Path) -> List[Tuple[str,
 class LintModuleTest:
    def __init__(self, test_file: Tuple[str, Path]) -> None:
        self._test_file = test_file


This is probably your IDE but your PRs keep removing white lines. This is just personal preferences, but I kinda like how white lines can separate different elements within a function.

Haha no, it's personal preference, imo compactness is good for readability, I think creating smaller code unit with functions also make more sense if something need to be separated. Anyway I'm going to stop doing that if you don't like it as we don't need churn around this.

doc/test_messages_documentation.py

doc/whatsnew/2/2.14/full.rst

Pierre-Sassoulas · 2022-06-05T21:07:49Z

There is a lot going on, with some of it just being general refactoring. Makes it quite hard to review this.

Right let's start by #6866

Pierre-Sassoulas · 2022-06-05T21:51:16Z

Then #6867. Even if we don't need to activate the old messages there was still some things to keep there.

So it's possible to to distinguish between genuine typoes and old configuration that could be cleaned. We use old_names to decrease message type to warning

Previousely they were considered unknown names.

github-actions · 2022-06-06T08:34:37Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on astroid:
The following messages are now emitted:

unknown-option-value:
Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/__init__.py#L91
unknown-option-value:
Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/node_classes.py#L9
unknown-option-value:
Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/nodes/__init__.py#L15

The following messages are no longer emitted:

bad-option-value:
Bad option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/__init__.py#L91
bad-option-value:
Bad option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/node_classes.py#L9
bad-option-value:
Bad option value for 'disable', expected a valid pylint message and got 'Ellipsis'
https://github.com/PyCQA/astroid/blob/main/astroid/nodes/__init__.py#L15

This comment was generated for commit c5f0ce1

Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 3, 2022

Pierre-Sassoulas added this to the 2.14.1 milestone Jun 3, 2022

Pierre-Sassoulas force-pushed the useless-option-value branch from 4968b40 to c48188a Compare June 3, 2022 13:52

Pierre-Sassoulas commented Jun 3, 2022

View reviewed changes

Pierre-Sassoulas requested a review from DanielNoord June 3, 2022 13:54

DanielNoord reviewed Jun 3, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Pierre-Sassoulas force-pushed the useless-option-value branch from 52911fd to 6b0df75 Compare June 3, 2022 14:51

This comment has been minimized.

Sign in to view

This was referenced Jun 4, 2022

[deleted messages] Refactor the constant to be able to give a reason #6826

Merged

[bad-option-value] Use the right confidence and refactor message #6829

Merged

Pierre-Sassoulas force-pushed the useless-option-value branch 2 times, most recently from 5f88740 to d21ce04 Compare June 4, 2022 15:53

Pierre-Sassoulas requested a review from DanielNoord June 4, 2022 15:54

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested a review from jacobtylerwalls June 4, 2022 16:59

jacobtylerwalls approved these changes Jun 4, 2022

View reviewed changes

.pyenchant_pylint_custom_dict.txt Outdated Show resolved Hide resolved

doc/whatsnew/2/2.14/full.rst Outdated Show resolved Hide resolved

jacobtylerwalls changed the title ~~Distinguish between genuine typoes and old configuration that could be cleaned in configuration warning~~ Distinguish between genuine typos and old configuration that could be cleaned in configuration warning Jun 4, 2022

Pierre-Sassoulas force-pushed the useless-option-value branch from 6d5b9c9 to d98b0ee Compare June 4, 2022 21:11

This comment has been minimized.

Sign in to view

Pierre-Sassoulas marked this pull request as draft June 5, 2022 06:53

Pierre-Sassoulas marked this pull request as ready for review June 5, 2022 07:30

DanielNoord reviewed Jun 5, 2022

View reviewed changes

doc/data/messages/u/useless-option-value/bad.py Show resolved Hide resolved

Pierre-Sassoulas force-pushed the useless-option-value branch from 621effb to 2559ee2 Compare June 5, 2022 08:03

Pierre-Sassoulas force-pushed the useless-option-value branch from b142c9d to d7fc85b Compare June 5, 2022 12:22

Pierre-Sassoulas force-pushed the useless-option-value branch from d7fc85b to adbceae Compare June 5, 2022 12:27

This comment has been minimized.

Sign in to view

Pierre-Sassoulas force-pushed the useless-option-value branch from adbceae to 50b1dc2 Compare June 5, 2022 14:51

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested a review from DanielNoord June 5, 2022 15:50

DanielNoord reviewed Jun 5, 2022

View reviewed changes

Pierre-Sassoulas mentioned this pull request Jun 5, 2022

Refactor bad option value stash in prevision of handling multiple type of messages #6866

Merged

Pierre-Sassoulas force-pushed the useless-option-value branch from 7973401 to e9b28c3 Compare June 5, 2022 21:30

Pierre-Sassoulas mentioned this pull request Jun 5, 2022

Print the launched command in doc examples tests #6867

Merged

Pierre-Sassoulas force-pushed the useless-option-value branch from e9b28c3 to 02b3d7c Compare June 5, 2022 21:50

This comment has been minimized.

Sign in to view

[bad-option-value] Add a 'useless-option-value' message

5927ed1

So it's possible to to distinguish between genuine typoes and old configuration that could be cleaned. We use old_names to decrease message type to warning

Pierre-Sassoulas force-pushed the useless-option-value branch from 02b3d7c to 18bb1ee Compare June 6, 2022 07:35

Pierre-Sassoulas requested a review from DanielNoord June 6, 2022 07:36

DanielNoord approved these changes Jun 6, 2022

View reviewed changes

[deleted messages] Fix helper function for old names of deleted messages

c5f0ce1

Previousely they were considered unknown names.

Pierre-Sassoulas force-pushed the useless-option-value branch from 18bb1ee to c5f0ce1 Compare June 6, 2022 07:53

Pierre-Sassoulas merged commit a5ca674 into main Jun 6, 2022

Pierre-Sassoulas deleted the useless-option-value branch June 6, 2022 08:38

Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 6, 2022

Pierre-Sassoulas mentioned this pull request Mar 16, 2023

unknown-option-value should result in non-zero exit code #8457

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish between genuine typos and old configuration that could be cleaned in configuration warning #6824

Distinguish between genuine typos and old configuration that could be cleaned in configuration warning #6824

Pierre-Sassoulas commented Jun 3, 2022 •

edited

Pierre-Sassoulas Jun 3, 2022

DanielNoord left a comment

DanielNoord Jun 3, 2022

Pierre-Sassoulas Jun 3, 2022

DanielNoord Jun 3, 2022

DanielNoord Jun 3, 2022

Pierre-Sassoulas Jun 3, 2022

DanielNoord Jun 3, 2022

DanielNoord Jun 3, 2022

DanielNoord Jun 3, 2022

Pierre-Sassoulas Jun 3, 2022

DanielNoord Jun 3, 2022

coveralls commented Jun 3, 2022 •

edited

This comment has been minimized.

Pierre-Sassoulas commented Jun 3, 2022

This comment has been minimized.

DanielNoord commented Jun 3, 2022

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

This comment has been minimized.

This comment has been minimized.

DanielNoord left a comment

DanielNoord Jun 5, 2022

Pierre-Sassoulas Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

This comment has been minimized.

github-actions bot commented Jun 6, 2022

Distinguish between genuine typos and old configuration that could be cleaned in configuration warning #6824

Distinguish between genuine typos and old configuration that could be cleaned in configuration warning #6824

Conversation

Pierre-Sassoulas commented Jun 3, 2022 • edited

Type of Changes

Description

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 3, 2022 • edited

Pull Request Test Coverage Report for Build 2446419315

💛 - Coveralls

This comment has been minimized.

Pierre-Sassoulas commented Jun 3, 2022

This comment has been minimized.

DanielNoord commented Jun 3, 2022

This comment has been minimized.

This comment has been minimized.

Pierre-Sassoulas commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

This comment has been minimized.

This comment has been minimized.

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Sassoulas commented Jun 5, 2022

Pierre-Sassoulas commented Jun 5, 2022

This comment has been minimized.

github-actions bot commented Jun 6, 2022

Pierre-Sassoulas commented Jun 3, 2022 •

edited

coveralls commented Jun 3, 2022 •

edited