pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.31k stars 1.14k forks source link

Provide an auto-upgrade option / migration tool for pylint configurations #5462

Open Pierre-Sassoulas opened 2 years ago

Pierre-Sassoulas commented 2 years ago

Current problem

There are breaking changes that could impact the configuration file and make our live as maintainer easier. Asking the user to upgrade their configuration files make most breaking chance impossible. I we can upgrade the configuration file automatically it's easier to make the configuration consistent and easier to maintain. For example a migration to pyproject.toml only instead of maintaining setup.cfg, pylintrc (ini) and pyrpoejct.toml.

Desired solution

New subcommand pylint --auto-upgrade-conf to upgrade the configuration file.

Additional context

https://github.com/PyCQA/pylint/issues/4741#issuecomment-984887726

The new framework for configuration file would help to implement this greatly, we would add an auto-upgraded expected result.

Talador12 commented 2 years ago

5465 mentions two examples that could be fixed by the above command. Definitely worth including these terminology updates as part of the --auto-upgrade-conf option

Pierre-Sassoulas commented 2 years ago

The auto-upgrade should remove pylint messages that do not exists anymore, or that can't be raised for interpreter below py-version from disable/enable.

Pierre-Sassoulas commented 2 years ago

The command that generate the toml config is doing that based on the current configuration. So we have a basis to improve upon. It's also going to need to have some tests before being deployed, and some specification after that to provide in-place upgrade.

Pierre-Sassoulas commented 1 year ago

Required for breaking changes like #6870. I'm going to start slow and build on it.

DanielNoord commented 1 year ago

Don't forget to integrate this in pylint-config!

Pierre-Sassoulas commented 1 year ago

I checked pylint-config, I think we used it to be able to reuse all of pylint options in order to generate the config from the existing one. There's a lot of complexity we still have following the refactor from optparse (pre-parsing etc.). It seems it would be a lot simpler to start from scratch and have only an argument for input conf / output conf / (maybe target pylint version ? Though I'd prefer not over-engineering the shit out of this upgrade tool and simply make all the upgrade for the latest pylint version at time of release). What do you think ? Is there an advantage in using the common parsing I don't see ?

DanielNoord commented 1 year ago

I checked pylint-config, I think we used it to be able to reuse all of pylint options in order to generate the config from the existing one. There's a lot of complexity we still have following the refactor from optparse (pre-parsing etc.). It seems it would be a lot simpler to start from scratch and have only an argument for input conf / output conf / (maybe target pylint version ? Though I'd prefer not over-engineering the shit out of this upgrade tool and simply make all the upgrade for the latest pylint version at time of release). What do you think ? Is there an advantage in using the common parsing I don't see ?

I think we originally also wanted to have support for both ini and toml and you kind of need to use the common parsing to build on an old config.

I'm not against a new tool, just think there should only be one "pylint configuration interaction tool". Happy to review refactors and PRs that work towards this as I do think that would indeed be an improvement!

Pierre-Sassoulas commented 1 year ago

Could we use the common parsing but skip the pre-processing of option and start from a fresh argparse parser ? After adding an upgrade subparser in _register_generate_config_options, the result of launching pylint-config is:

usage: pylint-config [options]

With no indication of the existing subcommand, when I would expect:

usage: PROG [-h] [--foo] {a,b} ...

positional arguments:
  {a,b}   sub-command help
    a     a help
    b     b help

Like in https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.add_subparsers.

I know it's probably possible to fix this in the current framework but I've been using argparse extensively since before it was a python builtin and I have no idea how without going on an adventure :smile:

DanielNoord commented 1 year ago

Could we use the common parsing but skip the pre-processing of option and start from a fresh argparse parser ?

I don't really understand this question.

After adding an upgrade subparser in _register_generate_config_options, the result of launching pylint-config is:

This should be relatively straightforward. I can create a PR that adds upgrade if necessary?

Pierre-Sassoulas commented 1 year ago

Could we use the common parsing but skip the pre-processing of option and start from a fresh argparse parser ?

I don't really understand this question.

We do some pre-processing in _preprocess_options in order to exit early for --version and the like, then we load plugins and add their parsing option to the parser automatically, etc.. I don't think it's useful when we want to upgrade the configuration, do you think we can extract the common parsing for ini/toml and skip that pre-processing altogether ?

After adding an upgrade subparser in _register_generate_config_options, the result of launching pylint-config is:

This should be relatively straightforward. I can create a PR that adds upgrade if necessary?

I've done the code to add 'upgrade' (could open a draft MR if it would help), the better default message is possible to make better right now though.

Consider the following code with a single subparser;

import argparse

def parse_args():
    parser = argparse.ArgumentParser(prog='PROG')
    parser.add_argument('--foo', action='store_true', help='foo help')
    subparsers = parser.add_subparsers(help='sub-command help')
    parser_a = subparsers.add_parser('a', help='a help')
    parser_a.add_argument('bar', type=int, help='bar help')
    # parser_b = subparsers.add_parser('b', help='b help')
    # parser_b.add_argument('--baz', choices='XYZ', help='baz help')
    parser.parse_args()

if __name__ == "__main__":
    parse_args()

This is the result when we use --help

usage: PROG [-h] [--foo] {a} ...

positional arguments:
  {a}         sub-command help
    a         a help

options:
  -h, --help  show this help message and exit
  --foo       foo help

But pylint config currently has this help:

usage: pylint-config [options]

I don't think we need to bother with this if we can decouple pylint-config from the pre-processing step though. I feel this would make our live easier (and the live of future contributors too).

DanielNoord commented 1 year ago

We do some pre-processing in _preprocess_options in order to exit early for --version and the like, then we load plugins and add their parsing option to the parser automatically, etc.. I don't think it's useful when we want to upgrade the configuration, do you think we can extract the common parsing for ini/toml and skip that pre-processing altogether ?

We do need the pre-processing to make sure the current config is valid, which we probably want to take into account when we are upgrading.

I don't think we need to bother with this if we can decouple pylint-config from the pre-processing step though. I feel this would make our live easier (and the live of future contributors too).

I'm not sure we can... It feels like the pre-processing is kind of crucial to our subsequent interpretation of configurations.

Pierre-Sassoulas commented 1 year ago

https://github.com/pylint-dev/pylint/pull/8702#discussion_r1198672394

Pierre-Sassoulas commented 1 year ago

After discussion on discord with @jacobtylerwalls the design could be :

A pylint subcommand with runtime check in pylint that print warning "have you run our --upgrade-config option?", and if we're worried about performance only show it once or something via caching to the same way we warn about the old cache directory

jacobtylerwalls commented 1 year ago

@Pierre-Sassoulas would you be alright with no longer considering this a release blocker? I don't think we should commit to a timeline if we don't have a PR.

Pierre-Sassoulas commented 1 year ago

Right, let's make this a blocker for 3.0.0 instead and add a note in the release note for 3.0.0a7 about this. (the stable doc PR took more time than I thought)