Skip to content

Conversation

@DanielNoord
Copy link
Owner

WIP.

Just to show how I would do the changes proposed in #15.

@Pierre-Sassoulas I'd still like to merge L19 and L20 (and possible create an ArgumentManager class to handle all of this a bit better), but with this approach (to me) it's much clearer where all options come from and where they get their value. By doing the default value parsing just before parsing all other configuration sources you know where any expected values could come from.

@DanielNoord DanielNoord added the enhancement New feature or request label Jan 6, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1662310150

  • 25 of 26 (96.15%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 99.502%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pydocstringformatter/utils/argument_parsing.py 19 20 95.0%
Totals Coverage Status
Change from base Build 1658405051: -0.5%
Covered Lines: 200
Relevant Lines: 201

💛 - Coveralls

Copy link
Collaborator

@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.

I see what you want to do now. It would work but I think using confuse (and as long as we have no config argparse's default argument) would be less verbose and require less code to maintain on our side. Did you check confuse :) ?

Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2022
@Pierre-Sassoulas
Copy link
Collaborator

Closing after cherry-picking and rebasing on #15

@Pierre-Sassoulas Pierre-Sassoulas deleted the make-every-formatter-optional-2 branch January 8, 2022 09:00
Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Jan 8, 2022
Pierre-Sassoulas added a commit that referenced this pull request Jan 8, 2022
* Add automatically generated options

* Refactor to avoid using a default value in argparse

See discussion here: #15 (comment)
And here #21

Co-authored-by: Daniël van Noord <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants