Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Pierre-Sassoulas opened this issue Dec 3, 2021 · 15 comments
Assignees
Labels
Configuration Related to configuration Enhancement ✨ Improvement to a component High effort 🏋 Difficult solution or problem to solve Work in progress

Comments

@Pierre-Sassoulas
Copy link
Member

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

#4741 (comment)

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

@Talador12
Copy link

Talador12 commented Dec 3, 2021

#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
Copy link
Member Author

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.

@DanielNoord DanielNoord moved this from To do: general to To do: related bugs in Migration to argparse May 10, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. label Jul 3, 2022
@Pierre-Sassoulas
Copy link
Member Author

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 Pierre-Sassoulas added this to the 3.0.0b1 milestone May 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas self-assigned this May 2, 2023
@Pierre-Sassoulas
Copy link
Member Author

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

@Pierre-Sassoulas Pierre-Sassoulas added Work in progress High effort 🏋 Difficult solution or problem to solve and removed Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. High effort 🏋 Difficult solution or problem to solve labels May 2, 2023
@DanielNoord
Copy link
Collaborator

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

@Pierre-Sassoulas Pierre-Sassoulas added the Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much label May 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a7 May 6, 2023
@Pierre-Sassoulas
Copy link
Member Author

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 ?

@pylint-dev pylint-dev deleted a comment from DanielNoord May 9, 2023
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented May 9, 2023

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
Copy link
Collaborator

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
Copy link
Member Author

#8702 (comment)

Pierre-Sassoulas added a commit that referenced this issue May 21, 2023
This will permit to parse config file without having to instantiate
a linter for tool working on pylint configuration like #5462
Pierre-Sassoulas added a commit that referenced this issue May 22, 2023
This will permit to parse config file without having to instantiate
a linter for tool working on pylint configuration like #5462
Pierre-Sassoulas added a commit that referenced this issue May 22, 2023
This will permit to parse config file without having to instantiate
a linter for tool working on pylint configuration like #5462
@Pierre-Sassoulas
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member Author

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)

@Pierre-Sassoulas Pierre-Sassoulas added Blocker 🙅 Blocks the next release and removed Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much labels Aug 15, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0a7, 3.0.0b1 Aug 15, 2023
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Oct 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0 Oct 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas removed the Blocker 🙅 Blocks the next release label Oct 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0, 3.0.1 Oct 2, 2023
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Oct 2, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.1, 3.1.0 Oct 4, 2023
Pierre-Sassoulas added a commit that referenced this issue Oct 12, 2023
)


In order to be able to automate the fix later in #5462
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 3.1.0 milestone Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Related to configuration Enhancement ✨ Improvement to a component High effort 🏋 Difficult solution or problem to solve Work in progress
Projects
No open projects
Development

No branches or pull requests

4 participants