Observe specific enables/disables even if followed by "all" #8819
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8819 +/- ##
==========================================
+ Coverage 95.89% 95.90% +0.01%
==========================================
Files 173 173
Lines 18511 18536 +25
==========================================
+ Hits 17751 17777 +26
+ Misses 760 759 -1
|
This comment has been minimized.
This comment has been minimized.
| @@ -147,3 +154,48 @@ def _config_initialization( | |||
| for arg in parsed_args_list | |||
| ) | |||
| ) | |||
|
|
|||
|
|
|||
| def _order_all_first(config_args: list[str], *, joined: bool) -> list[str]: | |||
There was a problem hiding this comment.
Did you consider making this part of ArgumentsManager? It feels like we want to make this the default behaviour and don't want it to be possible to parse arguments without this behaviour right?
There was a problem hiding this comment.
To be honest, I didn't, since I'm not clear what the responsibility of the ArgumentsManager is and didn't step through it with a debugger. It doesn't seem to do any argument parsing; it seemed that was done in _config_initialization. Happy to take suggestions though.
It feels like we want to make this the default behaviour and don't want it to be possible to parse arguments without this behaviour right?
Is there a way to avoid going through _config_initialization? π€
There was a problem hiding this comment.
We do so here:
pylint/pylint/pyreverse/main.py
Line 279 in f815e9f
Although
all isn't used here.
I also think we might skip in some test setups.
I'm not sure, we do quite a bit of parsing in config_initialization as well and perhaps ArgumentsManager should be kept small. Let's see what Pierre thinks.
There was a problem hiding this comment.
I'm also not very clear on the design if the argument parser, I usually have simple argparse add_argument calls with custom type if required in my code. The current design might be due to optparse being used before or the need to pre-parse some if the arguments ? Anyway, it seems to work as is, so I don't think committing too much in the current design or a refactor of the current design or this merge request would be worth it.
| @@ -43,6 +43,11 @@ | |||
| str(path.relative_to(FUNCTIONAL_DIR)) | |||
| for ext in ACCEPTED_CONFIGURATION_EXTENSIONS | |||
| for path in FUNCTIONAL_DIR.rglob(f"*.{ext}") | |||
| if (str_path := str(path)) | |||
| # The enable/disable all tests are not practical with this framework. | |||
| # They require manually listing ~400 messages, which will | |||
| @@ -147,3 +154,48 @@ def _config_initialization( | |||
| for arg in parsed_args_list | |||
| ) | |||
| ) | |||
|
|
|||
|
|
|||
| def _order_all_first(config_args: list[str], *, joined: bool) -> list[str]: | |||
There was a problem hiding this comment.
I'm also not very clear on the design if the argument parser, I usually have simple argparse add_argument calls with custom type if required in my code. The current design might be due to optparse being used before or the need to pre-parse some if the arguments ? Anyway, it seems to work as is, so I don't think committing too much in the current design or a refactor of the current design or this merge request would be worth it.
Type of Changes
Description
Let individual enables/disables take effect even if followed by
--enable=allor--disable=all. Previously, this was order-specific, which is tolerable on the CLI but inconvenient for configuration files.This means that for:
...you'll now get the fixme messages, whereas before, you'd have to reverse the order of the arguments. (If the new result is not what you want, you can get the prior behavior by removing the superfluous first argument.)
Order-dependence still applies for message categories such as "R", "W", etc. This PR just moves the
=allarguments to the front.Providing both
--enable=alland--disable=allin any order now raises an exception.Closes #3696