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

Improve handling of version flag #223

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Improve handling of version flag #223

merged 1 commit into from
Jul 14, 2023

Conversation

hhromic
Copy link
Contributor

@hhromic hhromic commented Jul 6, 2023

I noticed that when the destination struct is not versioned, the parser still accepts a --version flag but prints nothing.
This PR fixes the issue by implementing the following improvements to the version flag handling:

  • Only use/show builtin --version flag if args are versioned with a non-empty Version()
  • If args define a --version flag, honor it and disable/hide the builtin version flag
  • Only return ErrVersion when using the builtin version flag

This allows developers to freely define and use their own --version flag if they need in the destination struct.
Note that if args defines a --version flag, it always overrides the builtin version flag, even if args has Version().

I also split the TestVersion test into separate test for each versioning case.
Thanks for this great library, really like the API and simplicity!

@hhromic hhromic changed the title Only accept version flag if destination struct is versioned Improve handling of version flag Jul 6, 2023
@hhromic
Copy link
Contributor Author

hhromic commented Jul 6, 2023

Sorry for all the force pushes, I added extra tests after opening the PR. It is ready for review now :)

@hhromic hhromic mentioned this pull request Jul 14, 2023
@hhromic
Copy link
Contributor Author

hhromic commented Jul 14, 2023

I just discovered a use-case I was not aware before in this comment.
I will mark this PR as Draft until I figure out how to account for it too.

@hhromic hhromic marked this pull request as draft July 14, 2023 13:16
* Only use/show builtin `--version` flag if args are versioned with a non-empty `Version()`
* If args define a `--version` flag, honor it and disable/hide the builtin version flag
* Only return `ErrVersion` when using the builtin version flag
@hhromic
Copy link
Contributor Author

hhromic commented Jul 14, 2023

I think all is good now. Updated the description of this PR with the new behaviour. It should be fully backwards compatible with how the builtin version flag was used before (including manual parsing with ErrVersion) and still adding the improvements.

@hhromic hhromic marked this pull request as ready for review July 14, 2023 19:22
@alexflint
Copy link
Owner

Wonderful, thank you so much for doing this, and for making such a clean PR.

@alexflint alexflint merged commit 660b904 into alexflint:master Jul 14, 2023
@hhromic hhromic deleted the fix-version-flag branch July 14, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants