fix #423 : Add helper function and some documentation to parse shorthand go test flags. #424
Conversation
|
|
48f2d04 to
304a7e3
Compare
golangflag.go
Outdated
| // prefix of go test flags | ||
| const gotestFlagPrefix = "test." | ||
|
|
||
| // additionalGoTestFlagPrefix is optional: only the first value in additionalGoTestFlagPrefix will be used. |
There was a problem hiding this comment.
Really not a fan of this way to implement optionality - especially since the usage is for a string prefix that is concatenated onto the constant.
I could imagine just taking a (required) string and passing "" for the case when you don't want anything extra, but since this is an internal method and the only use case is whether to include a leading "-" or not, the method could even do that itself. In other words, the method could return strings.HasPrefix(s, "-test.") || strings.HasPrefix(s, "test.") and need no confusing optionality semantics.
I'm also a little bit uncertain of the naming; the point, imo, of breaking this out into a helper was to make it less tightly coupled with skipping just flags for go test (one might, for similar reasons, want to skip - or parse specifically - arguments for ginkgo, which are prefixed -ginkgo.).
Maybe we can call both the internal method and the exported one something more generic, focused on behavior rather than use case? How about flag.ParseSkippedFlags and shouldSkipFlag or something like that?
There was a problem hiding this comment.
I gave a shot at doing what you suggested. I also ventured in trying an alternative approach by adding a filed to FlagSet to make the whole skipping shorthand flags mechanism configurable if needed while preserving the default behavior as it was; you can have a look her valdar@ec3428a
…horthand go test flags.
There was a problem hiding this comment.
Thanks! I like this approach much better!
Wrt the extension mechanism I think that could be a nice add-on feature on this one, but let's do that separately so we don't have to block this on getting the design for that right.
For example, I could imagine that SetSkippedFlagPrefixes([]string) is a signature that's a bit unpractical to work with - if you want to add one, you have to know what the previously set ones were, but there's no way to query for that... So without someone with that need (I understand it you don't have that?) let's punt that down the line for now. I think the public API of this feature is generally phrased enough to support that if/when we want it, which is what we need to care about here.
Thanks for your contribution!
This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [github.com/spf13/pflag](https://github.com/spf13/pflag) | `v1.0.6` -> `v1.0.7` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>spf13/pflag (github.com/spf13/pflag)</summary> ### [`v1.0.7`](https://github.com/spf13/pflag/releases/tag/v1.0.7) [Compare Source](spf13/pflag@v1.0.6...v1.0.7) #### What's Changed - Fix defaultIsZeroValue check for generic Value types by [@​MidnightRocket](https://github.com/MidnightRocket) in spf13/pflag#422 - feat: Use structs for errors returned by pflag. by [@​eth-p](https://github.com/eth-p) in spf13/pflag#425 - Fix typos by [@​co63oc](https://github.com/co63oc) in spf13/pflag#428 - fix [#​423](spf13/pflag#423) : Add helper function and some documentation to parse shorthand go test flags. by [@​valdar](https://github.com/valdar) in spf13/pflag#424 - add support equivalent to golang flag.TextVar(), also fixes the test failure as described in [#​368](spf13/pflag#368) by [@​hujun-open](https://github.com/hujun-open) in spf13/pflag#418 - add support for Func() and BoolFunc() [#​426](spf13/pflag#426) by [@​LeGEC](https://github.com/LeGEC) in spf13/pflag#429 - fix: correct argument length check in FlagSet.Parse by [@​ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) in spf13/pflag#409 - fix usage message for func flags, fix arguments order by [@​LeGEC](https://github.com/LeGEC) in spf13/pflag#431 - Add support for time.Time flags by [@​max-frank](https://github.com/max-frank) in spf13/pflag#348 #### New Contributors - [@​MidnightRocket](https://github.com/MidnightRocket) made their first contribution in spf13/pflag#422 - [@​eth-p](https://github.com/eth-p) made their first contribution in spf13/pflag#425 - [@​co63oc](https://github.com/co63oc) made their first contribution in spf13/pflag#428 - [@​valdar](https://github.com/valdar) made their first contribution in spf13/pflag#424 - [@​hujun-open](https://github.com/hujun-open) made their first contribution in spf13/pflag#418 - [@​LeGEC](https://github.com/LeGEC) made their first contribution in spf13/pflag#429 - [@​ShawnJeffersonWang](https://github.com/ShawnJeffersonWang) made their first contribution in spf13/pflag#409 - [@​max-frank](https://github.com/max-frank) made their first contribution in spf13/pflag#348 **Full Changelog**: spf13/pflag@v1.0.6...v1.0.7 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS40My41IiwidXBkYXRlZEluVmVyIjoiNDEuNDMuNSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/794 Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org> Co-authored-by: Renovate Bot <bot@kriese.eu> Co-committed-by: Renovate Bot <bot@kriese.eu>
See issue #423