Migration to argparse from optparse

[Pierre-Sassoulas]

Current problem

optparse is deprecated since python 3.2 and is not developed anymore. Also, the parsing are handled in a strange way with callback that created stupid bug that could probably be avoided by refactoring the code (#3581) We need to migrate to another CLI parser, the most logical being argparse or click, but confuse is also a possibility because it permits to have default option in a configuration file that could be one of the configuration we already handle and make option / configuration consistent. I'm open to other libraries too.

Desired solution

No more optparse used in the code, small and modular handling of CLI argument parsing, no more special case, or minimal.

Additional context

#5152 (comment)
#5259 (comment)

[DanielNoord]

I think argparse makes the most sense as it is the direct successor to optparse.

To do this I think it makes the most sense to do some initial preparation, namely convert all options to a new class (we could use the one I proposed in #5152) and move them all to PyLinter. This avoids the current problem we're having with double settings etc and will probably also be faster (?) as we only need to parse arguments once instead for each individual checker.

[saroad2]

Even though argparse is a built-in library, and a way better solution than optparse, I strongly recommend using Click as your CLI library:

• It makes a shorter and cleaner code
• It has a lot of neat features that makes it so much easy to write CLI with.
• It is a living project that still publish new abilities and features
• It is supported by its developers
• It's widely used in the industry (personally, I use it all the time and in all of my projects)
• It includes testing tools that helps you test your CLI

[Pierre-Sassoulas]

I like click, but my concern is it adds a new dependency. Also argparse is compatible with confuse which would be very nice to help with consistency between options and configuration file enforced in a simple way.

[saroad2]

In my humble opinion, adding a dependency is not that of a problem. In the end it's all matter of "buy what you won't implement", using a library instead of developing something out of your domain. I think that click answers to this definition.

As to confuse, I am not familiar with this library so I don't have a strong opinion about it. In the end it's all matter of pros and cons. From my own experience with both argparse and click, I think click brings much more wholistic solution to the CLI programming process. But again, one an argue otherwise.

[DanielNoord]

I have started work on migration to argparse. I have not previous experience with any of the alternatives mentioned in this issue, but I think trying to depend only on stdlib is a good thing for a static checker such as pylint. It allows everybody to use pylint in combination with other linter or analysers.

One of the things that I think we should change (and I'm changing in my proposal) is that options are no longer registered to each individual checker class but to the PyLinter object. This is preparing for (eventually) supporting sub-directory configuration:
If we create helper functions that can be passed an argparse.Namespace (or look up the correct Namespace) and return the current value of an option, we can create Namespaces for individual directories. This is similar to how we're already using get_global_option. We would just need to enforce the use of these helpers more strictly, but I think that approach might work (and fix a long standing issue with pylint).

[DanielNoord]

@Pierre-Sassoulas I want to ask for input before committing to this:
Do we want to keep requiring the sections in options files? It adds a lot of extra checks in the code and honestly I don't see why we should require it. It's trivial to keep supporting it but I would like to support:

[tool.pylint]
missing-member-hint = true

[tool.pylint."TYPECHECK"]
missing-member-hint = true

[tool.pylint."I-ORDER-MY-SECTIONS-THE-WAY-I-WANT-TO"]
missing-member-hint = true

I do not immediately see what having these sections internally offers to us.

[Pierre-Sassoulas]

I do not immediately see what having these sections internally offers to us.

Only upside would be to have the same option within two checkers with a different value in each checkers ? Imo not an upside and more like a nightmare. Otherwise I don't know. I think we expose our internals checker name to users so it makes it more complex for us to change anything in the checkers and more complex for users to understand what they need to add in the configuration. See #3181, with currentely 37 upvotes from confused configuration writers. One of the top upvoted issues we have.

Imo, the only reason it stays this way is the enormous amount of existing config and doc about config in the wild. I agree with you that we should support all three possibilities, maybe not take the sections into account at all. This is the kind of thing a migration tool would fix / clean up / normalize to:

[tool.pylint]
missing-member-hint = true

[DanielNoord]

I'll go ahead with that implementation then! 😄

[DanielNoord]

To give a little status update:
Almost all changes required to use argparse alongside optparse have now been made. There are a couple of PRs still in the pipeline (most notable moving pyreverse to argparse), but the biggest changes have already been made.
One last step before the implementation is complete is the drop of 3.7 support. Since re.Pattern is only a class since 3.7 I can only use isinstance(value, re.Pattern) from that version onwards. Using hasattr(value, "pattern") seems a bit of a waste considering we are dropping support in less than a week.

Thus, this project should slow down during the second half of this week and mostly continue next week.

After making sure we are using linter.namespace instead of linter.config internally everywhere I'm going to work on getting API parity between OptionsProviderMixIn and ArgumentsProvider.
After that the last step is to check whether we can simply replace linter.config with linter.namespace and be done with it. That would make it much easier for plugins to migrate and I think there is large chance that this should be possible. We just need to handle things carefully.

Edit:
API parity between OptionsManagerMixIn and ArgumentsManager is now almost completed.

[DanielNoord]

With #6278 almost merged we're nearing the process of completion on this particular issue. After that there are still many more issues on the project that should be merged before releasing 2.14, but I think we are nearing a state in which we could potentially do a pre-release to see if I haven't broken anything.

What's left to do:

• Stop parsing with optparse completely
• Use config instead of namespace again in all of our own code
• Remove namespace attribute in favour of config on ArgumentsManager

[DanielNoord]

I'm running into a problem with the migration. Currently we still have linter.config which is optparse.Values and linter.namespace which is argparse.Namespace.

Internally we only use the namespace attribute as that is where we now store and record all of our options on. This works well for us, but if there are any plugins that want to interact with our options this not backwards-compatible. In our documentation we have/had the explicit example of doing linter.config.ignore += ("bin",).
Since we don't read config.ignore anymore any plugin that changes the config attribute does nothing meaningful.

That's why in the post above I talk about replacing namespace with config again. Now that argparse is "complete" we can almost transition linter.config to be argparse.Namespace and make such plugins compatible again. (Due to the nature of both Values and Namespace I think most plugins should be able to work with both without any change).

However, this requires on change which might be backwards incompatible: removing all calls to methods that depend on linter.config being optparse.Value. See for example #6293. Without changes like that we will never be able to make linter.config a Namespace object (I think).

As I see it there are two options:

1. Not going forward with the change from .namespace to .config and potentially breaking any plugin that directly interacts with .config
2. Removing all calls to methods that require .config to be Options. These methods are mostly "private" methods anyway: I don't think plugins are likely to re-call a function that parses a configuration file. It's just that they are not underscored so we can't remove them immediately.

I'd be in favour of option 2 but I would like input from others!

[Pierre-Sassoulas]

I think we should rename namespace to config as you said or we'll still be impacted by optparse in 2030. Ideally we would want a solution that do not need to have two branch depending on the pylint version in libraries depending on pylint's code but if it's not possible, well...

We could raise an error explaining what needs to be done to lib mainteners instead of simply removing the function, I think it's better than outright getting a no-member without explanation (then we really remove it in 3.0).

[DanielNoord]

Do you want to go as far as changing ArgumentsManager.load_command_line_configuration (for example) to become an empty shell with a DeprecationWarning and a longer message explaining what anybody should be doing?
Or would you still want to keep those methods even though they might only partially work?

[Pierre-Sassoulas]

to become an empty shell with a DeprecationWarning

I think we should at least try to make it work the old function but if it's too much work then a deprecation warning is misleading as it's not a deprecation: it does not work anymore. We'd need to crash instead of silently not doing what the code is supposed to do. But then we need to release 3.0 as we're breaking compatibility...

[DanielNoord]

• Stop parsing with optparse completely
• Use config instead of namespace again in all of our own code
• Remove namespace attribute in favour of config on ArgumentsManager

With #6319 this will be done.

Two last tasks remain for this to be closed and a pre-release to be releasable:

• Fix any outstanding coverage issues in pylint/config
  • Exclude deprecated methods and files from coverage #6329
    * [ ] pylint/config/init.py
  • pylint/config/argument.py > Improve coverage of argparse #6332
  • pylint/config/arguments_manager.py > Increase coverage for argparse #6331
  • pylint/config/config_file_parser.py > Improve coverage of argparse #6332
  • pylint/config/utils.py > Increase coverage for argparse #6331
    * [ ] pylint/config/find_default_config_files.py
• Go through all TODOs and see if any need immediate fixing
• Final look at what is and what isn't public API.
• Deprecate last unused classes after pyreverse transition

[DanielNoord]

I'm going to leave the lower coverage in __init__ and find_default_config_files.py as those are not related to the recent changes. I'd like to finish this as soon as possible.

Updating the TODO above.

[DanielNoord]

One final TODO list and we're done with this issue:

• Transition pyreverse from optparse to argparse #6334
• Make sure we don't use deprecated classes in our tests

[DanielNoord]

Going to close this as there is nothing left to do here 😄

[jacobtylerwalls]

A lot of useful changes came from this project, but I just wonder if @DanielNoord has any insight into the proposal to undeprecate optparse.

[DanielNoord]

Yeah I saw that: they should definitely undeprecate it!

I think the main benefit we got out of this that it forced me to redo most of our configuration handling which 1) gave me a lot of insight into the codebase and ideas to improve it and 2) forced us to refactor and standardize some undefined behaviour.
argparse itself hasn't been that useful compared to optparse. Both are really not that user friendly to work with and lead to weird behaviour (as described in the issue).

I'm glad that they deprecated it as it gave us a reason to have a good look at this code, but as to argparse being any better than optparse that would be a definitive +0 from me.