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

Create new Argument class for options #5152

Closed
wants to merge 1 commit into from

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

I've looked into argparse and the move from optparse towards it. This is still a long road ahead. We would need to rewrite (almost) every bit of code dealing with options and config.

I've thought about the best way to do this and think the best way might be to start preparing the codebase by duplicating some functionality/data and slowly preparing a final move. It helps that the terminology of argparse relies on arguments instead of options, which allows duplicating methods and attributes with similar though different names.
Before continuing with this I would like some response from maintainers if this is indeed the best way to do this, or if there are other ways to allow for a gradual change to argparse.

This PR is a first step towards this gradual transformation. I have created a new NamedTuple and one that inherits from it. These are based on the parameters we would eventually need to pass to ArgumentParser.add_argument(). For now I would suggest to add duplicate all options of all checkers into an arguments attributes. If all checkers have an arguments attribute we can then start working on methods that utilise this (for example the documentation creation methods).
The StoreArgument is probably not the only type of subclassed NamedTuple I will need to create to do this, but serves as an example of how subclassing Argument would work.

TLDR: 1) Is temporary duplication the best way to facilitate the move towards argparse? 2) Is the use of Argument NamedTuples's a good way to store arguments data?

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Oct 13, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1339360339

  • 21 of 21 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.222%

Totals Coverage Status
Change from base Build 1337381471: 0.01%
Covered Lines: 13602
Relevant Lines: 14591

πŸ’› - Coveralls

@cdce8p
Copy link
Member

cdce8p commented Oct 13, 2021

I would suggest to focus on 2.12 for now and look at this only afterwards, ie. for 2.13. Still some other things to sort out first.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Oct 14, 2021
@Pierre-Sassoulas
Copy link
Member

Thank you for starting this, this is indeed an enormous refactor that we need to do before python 3.12 if I remember correctly. I have some ideas about it but I agree with @cdce8p let's talk about it later.

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Oct 16, 2021
@DanielNoord DanielNoord mentioned this pull request Oct 25, 2021
4 tasks
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 4, 2021

@Pierre-Sassoulas Seeing your recent issue (#5259) also mentions the change to argparse it seems we're slowly building up a list of issues/enhancements that are waiting or depending on the move to argparse. You said you and @cdce8p had some thoughts about this as well so it might be time to start aggregating those and create a clear roadmap to follow.

I'm not sure how you did this for previous big changes but making an issue with a to-do list or even a project might allow me and other contributors to help the process along!

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Dec 3, 2021
@DanielNoord
Copy link
Collaborator Author

Closing this in favour of #5584 which I believe is a much better approach.

@DanielNoord DanielNoord deleted the optparse branch December 21, 2021 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants