enable/disable options in rcfile should not depend on the order with which they are specified

[sandrotosi]

Hello,
i'm running pylint 2.5.3 / astroid 2.4.2; i kinda understand why

$ pylint --enable=all --disable=fixme

behaves differently than

$ pylint --disable=fixme --enable=all

(where the first command enables everything and then disable fixme, while the second command the disable option is overwritten by enable=all) but i dont think it should be the same in the rcfile: the disable section is (by default) before the enable section, so if i want to have the same effect of command1 i need to swap the sections around.

on the cli i can specify multiple enable/disable options, but that's not allowed in the rcfile, so the
current result is extremely counter-intuitive; and rcfile with

disable=fixme
enable=all

is clear what result the user wants: to enable all checks except for some, and i shouldnt need to move config options around.

can you please get that fixed?

Thanks!

[kasium]

+1 from my side. Same goes for the pyproject.toml file

[Pierre-Sassoulas]

This should be done during a configuration refactor, or it will be a small patch on something old and already very patched using optparse.

[kasium]

I tried to figure out where exactly this order sensitive disable/enable comes from since it affects CLI, init and toml config. Can you point me to the right place? I can try to open a PR

[Pierre-Sassoulas]

Everything happens in Pylinter and I think the relevant class with the behavior for options parsing is config.OptionsManagerMixIn, (without absolute certainty). I would be interested in what you think of the current configuration and what you would do differentely if you take the time to understand how it works :)

[kasium]

Thanks for the hint. As far as I can say, the behavior comes from this code:
https://github.com/PyCQA/pylint/blob/a12242b2f44d5244bd1cacc6bc78df8c9c8e4296/pylint/config/option_manager_mixin.py#L309-L318

parser.items(section) is a list (so it's sorted), so the definition order is important. To be honest I'm not sure how to fix this since a very general loop is used

[Pierre-Sassoulas]

I can't think of a solution that will keep this for loop in its current state either.

[kasium]

Are there any plans to refactor the configuration module to use argparse and to modernize it a little bit?
Else, I'm not quite sure how to proceed. I guess this would be a breaking change, because

• disable=all ->enable=bar = bar
• enable=bar -> disable=all = all

But the other way round, we also miss scenarios:

• enable=all ->disable=bar = all except bar
• disable=bar -> enable=all = all.

So both orders have up and downsides. A possible statement could be: "It's not a bug, it's a feature 😄 "

Else maybe something like this (quite hacky and we need to decide on an order

[...]
order_sensitive_config = {}
if option in ["enable", "disable"]:
  order_sensitive_config[option] = value
  continue
try: 
  self.global_set_option(option, value) 
except (KeyError, optparse.OptionError): 
   continue
[...]
if "enable" in order_sensitive_config:
  self.global_set_option("enable", order_sensitive_config["enable"]) 
if "disable" in order_sensitive_config:
  self.global_set_option("disable", order_sensitive_config["disable"]) 

[Pierre-Sassoulas]

I think the logical order is to always take all into account first, then the other one to enable or disable some of the message. Ie:

disable=all ->enable=bar = only bar
enable=bar -> disable=all = only bar
enable=all ->disable=bar = all except bar
disable=bar -> enable=all =all except bar

My reasoning is that you would not specify disable or enable when you use all for the other without wanting to modify the behavior of all. Said another way, if enable=bar comes first it's effect will be completely canceled by disable=all, whereas the contrary is not true: all still has an effect on all other message than bar. So the user probably expect all to be taken into account first as they're not adding option for the sake of it.

You're right that it can be considered a breaking change, maybe we could move that to 3.0, or simply call that a bug fix, it depends on how "intuitive" this solution really is (maybe I've convinced myself but it will not be understood this way at all ?). Let me know what you think.

Are there any plans to refactor the configuration module to use argparse and to modernize it a little bit?

There are plans (https://github.com/PyCQA/pylint/projects/1) but this is not a priority for most contributors and as a maintainers most of our time consists of reviewing merge requests, triaging issues and implementing absolutely necessary changes like the new match pattern in python 3.10, so there's very little time for this kind of (very important) refactoring project.

[kasium]

Thanks a lot for this insights.

I agree, it depends a lot how you look on this: It's either a breaking change or a bug fix. Since you asked what I would do: Personally, I would not use "dynamic" loops at all, but handle the options one by one. This leads obviously to more code, but options order dependencies and so in will disappear. But this is not possible w/o a refactoring.

So, what do you think about my approach above? It's not very nice, but I have o other idea. I like the idea to handle "all" special; this can be easily added.

[Pierre-Sassoulas]

Thank you. There's I think foor aspects to the refactor :

• The loop is too generic. I agree with what you said.
• optparse is very old and would benefit from an argparse revamp
• Inheritance is used so everything is mixed up in Pylinter and even some checkers (composition and a class to handle the configuration, and the configuration only, would help imho)
• There's call back code for some options which means the options parsing is all other the place and hard to understands

If you can refactor only this for loop without modifying the other aspect that's fine for me, but a preliminary refactor might help. In particular I think argparse design force each argument to be handled separately, and this for loop would simply disappear (?) as well as a lot of optparse code that you don't need to do anymore nowadays. I don't know about click or confuse but this would be another possibility.

[kasium]

To be honest, moving away from optparse to argparse/click (click is btw very nice) would be (as far as I can say) a very drastic change. I did only one small change so far in pylint, so I'm kind of afraid doing this change. As you said, there are many patches and callbacks making it hard to understand for beginners. In the end, the code is not even typed, which add just more complexity and hurdles.

Any idea how we can move on from here?

Maybe a complete drop of the old code with a fresh design could help?

[Pierre-Sassoulas]

Maybe a complete drop of the old code with a fresh design could help?

Yes, that would make a lot of sense especially for the transition to click or argparse. I think it's a lot easier to reproduce pylint's arg parsing from zero then replace the original code than to progressively refactor. It would be a breaking change though. I'm pretty sure that some strange bugs due to everything being done by hand with optparse are considered feature now.

[Pierre-Sassoulas]

I spoke about confuse too, because it could help define a template yaml and have the argument to parse done automatically.

https://confuse.readthedocs.io/en/latest/

Integration with command-line arguments via argparse or optparse from the standard library. Use argparse’s declarative API to allow command-line options to override configured defaults.