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
Call FlagStringer in String() method of slice flags #1508
Conversation
The default help template relies on the String() method of Flag to render the flag. For most flag types, String() indirects through FlagStringer, so that is the best place to customize flag rendering. FlagStringer was not called for slice flags because their help output differs from other flags in two ways: there can be multiple default values, and the flag name is shown two times to indicate that the flag can be specified multiple times. To make multiple values work in the FlagStringer, I simply changed GetValue() to return all values. Showing the flag more than once is achieved through a new interface, DocGenerationSliceFlag, which the FlagStringer uses to decide whether the flag is a slice flag type.
Some more context about this PR. We use a custom
When we added a StringSliceFlag (
This PR provides me with a way to fix the issue on our side. With the changes applied, our output looks aligned by default, and I can add custom formatting for slice flags. |
If the new interface type is not wanted, I'm also happy to port this patch to the v3 branch and extend the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fjl Thank you so much for this.
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/g-rath/osv-detector](https://togithub.com/g-rath/osv-detector) | require | minor | `v0.7.2` -> `v0.8.0` | | [github.com/urfave/cli/v2](https://togithub.com/urfave/cli) | require | minor | `v2.17.1` -> `v2.19.2` | | [golang.org/x/crypto](https://togithub.com/golang/crypto) | require | digest | `4161e89` -> `56aed06` | | [golang.org/x/exp](https://togithub.com/golang/exp) | require | digest | `b9f4876` -> `4de253d` | --- ### Release Notes <details> <summary>g-rath/osv-detector</summary> ### [`v0.8.0`](https://togithub.com/G-Rath/osv-detector/releases/tag/v0.8.0) [Compare Source](https://togithub.com/g-rath/osv-detector/compare/v0.7.2...v0.8.0) #### What's Changed - support parsing `poetry.lock`, for Python ([G-Rath/osv-detector#156) - support parsing `pubspec.lock`, for Dart ([G-Rath/osv-detector#159) **Full Changelog**: G-Rath/osv-detector@v0.7.2...v0.8.0 </details> <details> <summary>urfave/cli</summary> ### [`v2.19.2`](https://togithub.com/urfave/cli/releases/tag/v2.19.2) [Compare Source](https://togithub.com/urfave/cli/compare/v2.19.1...v2.19.2) #### What's Changed - fix: stop automatic sorting for --help by [@​FGYFFFF](https://togithub.com/FGYFFFF) in [urfave/cli#1430 #### New Contributors - [@​FGYFFFF](https://togithub.com/FGYFFFF) made their first contribution in [urfave/cli#1430 **Full Changelog**: urfave/cli@v2.19.1...v2.19.2 ### [`v2.19.1`](https://togithub.com/urfave/cli/releases/tag/v2.19.1) [Compare Source](https://togithub.com/urfave/cli/compare/v2.19.0...v2.19.1) #### What's Changed - Fix:(issue\_1500). Fix slice flag value duplication issue by [@​dearchap](https://togithub.com/dearchap) in [urfave/cli#1502 **Full Changelog**: urfave/cli@v2.19.0...v2.19.1 ### [`v2.19.0`](https://togithub.com/urfave/cli/releases/tag/v2.19.0) [Compare Source](https://togithub.com/urfave/cli/compare/v2.18.2...v2.19.0) #### What's Changed - Fix:(issue\_1505) Fix flag alignment in help by [@​dearchap](https://togithub.com/dearchap) in [urfave/cli#1506 **Full Changelog**: urfave/cli@v2.18.2...v2.19.0 ### [`v2.18.2`](https://togithub.com/urfave/cli/releases/tag/v2.18.2) [Compare Source](https://togithub.com/urfave/cli/compare/v2.18.1...v2.18.2) #### What's Changed - Configure GenericFlag's Destination type as struct not pointer by [@​nkuba](https://togithub.com/nkuba) in [urfave/cli#1442 #### New Contributors - [@​nkuba](https://togithub.com/nkuba) made their first contribution in [urfave/cli#1442 **Full Changelog**: urfave/cli@v2.18.1...v2.18.2 ### [`v2.18.1`](https://togithub.com/urfave/cli/releases/tag/v2.18.1) [Compare Source](https://togithub.com/urfave/cli/compare/v2.18.0...v2.18.1) #### What's Changed - Ensure "generate" step runs in CI prior to diff check by [@​meatballhat](https://togithub.com/meatballhat) in [urfave/cli#1504 **Full Changelog**: urfave/cli@v2.18.0...v2.18.1 ### [`v2.18.0`](https://togithub.com/urfave/cli/releases/tag/v2.18.0) [Compare Source](https://togithub.com/urfave/cli/compare/v2.17.2...v2.18.0) #### What's Changed - Call FlagStringer in String() method of slice flags by [@​fjl](https://togithub.com/fjl) in [urfave/cli#1508 #### New Contributors - [@​fjl](https://togithub.com/fjl) made their first contribution in [urfave/cli#1508 **Full Changelog**: urfave/cli@v2.17.2...v2.18.0 ### [`v2.17.2`](https://togithub.com/urfave/cli/releases/tag/v2.17.2) [Compare Source](https://togithub.com/urfave/cli/compare/v2.17.1...v2.17.2) #### What's Changed - Remove nonexistent phony targets by [@​meatballhat](https://togithub.com/meatballhat) in [urfave/cli#1503 - wrap: Avoid trailing whitespace for empty lines by [@​abitrolly](https://togithub.com/abitrolly) in [urfave/cli#1513 #### New Contributors - [@​abitrolly](https://togithub.com/abitrolly) made their first contribution in [urfave/cli#1513 **Full Changelog**: urfave/cli@v2.17.1...v2.17.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/google/osv.dev). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMjIuMyIsInVwZGF0ZWRJblZlciI6IjMyLjIzNC4yIn0=--> Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
What type of PR is this?
What this PR does / why we need it:
The default help template relies on the
String()
method of Flag to render the flag. For most flag types,String()
indirects throughFlagStringer
, so that is the best place to customize flag rendering.FlagStringer
was not called for slice flags because their help output differs from other flags in two ways: there can be multiple default values, and the flag name is shown two times to indicate that the flag can be specified multiple times.To make multiple values work in the
FlagStringer
, I simply changedGetValue()
to return all values. This should be OK since the purpose of GetValue() is rendering documentation.Showing the flag more than once is achieved through a new interface,
DocGenerationSliceFlag
, which theFlagStringer
uses to decide whether the flag is a slice flag type. I'm not entirely happy with adding a new interface, butit seems like the only fully backwards-compatible solution.