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

Add SliceFlag wrapper and fix bugs in existing implementations #1409

Merged
merged 2 commits into from Jun 18, 2022

Conversation

joeycumines
Copy link
Contributor

@joeycumines joeycumines commented Jun 6, 2022

What type of PR is this?

  • bug
  • feature

What this PR does / why we need it:

The SliceFlag implementation and associated aliases (MultiStringFlag, etc) extend the existing slice implementations (StringSliceFlag, etc) to support actual slices as the flag value and destination.

This change also fixes various bugs in the existing implementation. Notably, the StringSliceFlag.Apply implementation would modify the input (default) Value, if an env var was set, and no destination was provided. The bugs fixed in the other three implementations were all already fixed in either StringSliceFlag, or in one case (ignoring empty env var) in Float64SliceFlag.

Which issue(s) this PR fixes:

Fixes #1410

Special notes for your reviewer:

I'm not sure if the "ignoring empty env var" behavior is desirable or not. It seems like it should be made consistent, at least.

Release Notes

Add SliceFlag and aliases (MultiStringFlag, MultiIntFlag, MultiFloat64Flag, MultiInt64Flag).
Fix value/destination behavior for slice flags.

The SliceFlag implementation and associated aliases (MultiStringFlag, etc)
extend the existing slice implementations (StringSliceFlag, etc) to support
actual slices as the flag value and destination.

This change also fixes various bugs in the existing implementation. Notably,
the StringSliceFlag.Apply implementation would modify the input (default)
Value, if an env var was set, and no destination was provided. The bugs fixed
in the other three implementations were all already fixed in either
StringSliceFlag, or in one case (ignoring empty env var) in Float64SliceFlag.
@joeycumines joeycumines requested a review from a team as a code owner Jun 6, 2022
@meatballhat meatballhat added kind/bug describes or fixes a bug area/v2 relates to / is being considered for v2 labels Jun 10, 2022
@meatballhat meatballhat self-assigned this Jun 10, 2022
@meatballhat meatballhat added the kind/feature describes a code enhancement / feature request label Jun 10, 2022
@meatballhat
Copy link
Member

meatballhat commented Jun 10, 2022

@joeycumines This is amazing! Thank you so much for taking this on!

Here comes the sad part. As much as I'm excited about the possibility of using generics, we won't be able to do this in the 2.x series until go 1.20 is released. Possible paths forward include:

  • target the 3.x series
  • rework this PR to drop use of generics entirely
  • rework this PR to leave the generics in place, but targeting only go 1.18 (add //go:build 1.18), and separately implementing a generic-less version with an inverted build constraint (//go:build !1.18)
  • other options?

@joeycumines
Copy link
Contributor Author

joeycumines commented Jun 12, 2022

Thanks @meatballhat :)

I've added a build constraint + restructured it slightly, so it should no longer impact support for older versions of Go.

@joeycumines
Copy link
Contributor Author

joeycumines commented Jun 18, 2022

Could I get an update on getting this merged?

I don't want to forget I have this PR open, is all :)

Looks like it passes CI now

@meatballhat
Copy link
Member

meatballhat commented Jun 18, 2022

@joeycumines Sorry for my delay 😭 This week was 1000% more kiddo chaos than usual for me. Weekend mode: engage 🤘🏼

Copy link
Member

@meatballhat meatballhat left a comment

@joeycumines 🤯 Wow. Very wow.

// applyFlagValueHook wraps calls apply then wraps flags to call a hook function on update and after initial apply.
func applyFlagValueHook(set *flag.FlagSet, apply func(set *flag.FlagSet) error, hook func()) error {
if apply == nil || set == nil || hook == nil {
panic(`invalid input`)
Copy link
Member

@meatballhat meatballhat Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a library and this function already has an error return signature, I think I'd rather this not panic. WDYT?

Copy link
Contributor Author

@joeycumines joeycumines Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, panics are ideal for "misuse of API" cases, especially internal / unexported APIs, as, generally speaking, panics are more likely to be noticed, and you want to notice any such cases. The least-effort alternative is just having it panic once it tries to call a nil function, which is similar, but more likely to go unnoticed.

If there was a use case for having it return an error, that could always be added later.

@meatballhat meatballhat merged commit 8007c54 into urfave:main Jun 18, 2022
13 checks passed
meatballhat added a commit that referenced this issue Jun 18, 2022
Update the v2 docs since merging #1409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug kind/feature describes a code enhancement / feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants