Skip to content

Use argparse config handler on three checkers #6124

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2022
Merged

Use argparse config handler on three checkers #6124

merged 1 commit into from
Apr 5, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
✓ 🔨 Refactoring

Description

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Apr 2, 2022
@DanielNoord DanielNoord added this to the 2.14.0 milestone Apr 2, 2022
@DanielNoord DanielNoord added the Blocked 🚧 Blocked by a particular issue label Apr 2, 2022
@DanielNoord
Copy link
Collaborator Author

Blocked by #5921.

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Apr 2, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do the migration from python 3.6 now then.

@DanielNoord
Copy link
Collaborator Author

Let's do the migration from python 3.6 now then.

We're still merging hot fixes. Still is a fairly trivial PR. I would be okay with waiting a bit. There is enough else for me to do!

@Pierre-Sassoulas
Copy link
Member

OK, after https://github.com/PyCQA/pylint/milestone/61 is closed then ?

@DanielNoord
Copy link
Collaborator Author

OK, after https://github.com/PyCQA/pylint/milestone/61 is closed then ?

I'd say, let's wait a minimal week since last hotfix release. That gives people ample time to report any new issues. I agree with Marc, the change to 3.7 is likely going to make releasing hotfixes much more difficult.

@Pierre-Sassoulas
Copy link
Member

last hotfix release.

We continuously release false positive fixes now so they won't be a last hotfix release. Do you mean the last one with crashes fixed inside it ?

releasing hotfixes much more difficult.

I agree too, but the question is when do we release 2.14 and not ever hot fix python 3.6 anymore :) Depends on how blocked we are. There are maybe 4 MR now and 2.14 is close to 100 issues already (lots of documentation but still). So, I'm already considering releasing 2.14.

@DanielNoord
Copy link
Collaborator Author

We continuously release false positive fixes now so they won't be a last hotfix release. Do you mean the last one with crashes fixed inside it ?

Yeah, or false positives introduced by 2.12.

I agree too, but the question is when do we release 2.14 and not ever hot fix python 3.6 anymore :) Depends on how blocked we are. There are maybe 4 MR now and 2.14 is close to 100 issues already (lots of documentation but still). So, I'm already considering releasing 2.14.

If possible I would wait with 2.14 until the argparse migration has been finished. That way we can do a "light" release with a move to 3.7 + a request for plugin maintainers on feedback on the new options handling. Looking at the speed at which it's going now I think that it is feasible to do so.
Only 227 references to .config remain in the codebase right now.

@DanielNoord DanielNoord changed the title Use argparse config handler on five checkers Use argparse config handler on foure checkers Apr 5, 2022
@DanielNoord DanielNoord changed the title Use argparse config handler on foure checkers Use argparse config handler on four checkers Apr 5, 2022
Pierre-Sassoulas
Pierre-Sassoulas requested changes Apr 5, 2022
View reviewed changes
pylint/checkers/base/name_checker/checker.py Outdated
Comment on lines 303 to 305
custom_regex = getattr(
self.linter.namespace, custom_regex_setting_name, None
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle this in the option parsing directly and not later with getattr ? I think it's not ideal as it's a delayed default value that put a little of option parsing inside checkers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These options are created dynamically for each name type we have.

We can probably define them in checker.options but that's more a general refactor of this checker. By creating them dynamically we do remove a lot of duplication of the code though.

@DanielNoord DanielNoord changed the title Use argparse config handler on four checkers Use argparse config handler on three checkers Apr 5, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2094531864

  • 15 of 16 (93.75%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0007%) to 94.522%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/checkers/spelling.py 8 9 88.89%
Totals Coverage Status
Change from base Build 2094485699: 0.0007%
Covered Lines: 15701
Relevant Lines: 16611
💛 - Coveralls

@DanielNoord
Copy link
Collaborator Author

Missing coverage is also missing on main so I'm merging.

@DanielNoord DanielNoord merged commit 48e6585 into pylint-dev:main Apr 5, 2022
@DanielNoord DanielNoord deleted the argparse-spelling branch April 5, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Blocked 🚧 Blocked by a particular issue Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.
3 participants
@DanielNoord @Pierre-Sassoulas @coveralls