- Notifications
You must be signed in to change notification settings - Fork6
Fix overwrite by SetDefault for options that share Value#23
Conversation
| if opt.ValueSource != ValueSourceNone { | ||
| continue | ||
| } |
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.
Review: This was removed in this refactor because we have a different way of handling values that have been set by the user.
However, this breaksone test in coder/coder:https://github.com/coder/coder/actions/runs/11782339194/job/32817210417?pr=15476#step:7:621
I could add aif opt.Value == nil && opt.ValueSource != ValueSourceNone { continue } here and it will fix the test. The only problem is, I don't understand the logic. If the value has been provided by the user andopt.Value == nil, that's OK. But if the value hasn't been provided, then we error out. Why?
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.
I haven't spent time to try and figure this out. This felt vaguely familiar, so I looked up when it was added.
It was added in this commit:coder/coder@7edea99
And this PR:coder/coder#6934
So it has to relate with YAML support somehow? I was hoping the commit when it was added would have more context.
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.
Thanks for looking and sharing, that was very helpful! It lead me to this:e053fda (#23).
| err = makeCmd("def.com", "").Invoke("--alt-url", "hup").Run() | ||
| require.NoError(t, err) | ||
| require.Equal(t, "hup", got) |
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.
Should we try to handle cases where both are set? Currently it defaults to the last one set. Should we at least make this deterministic, like by alphabetical order?
I have not tested if 1 is set with an env var, the other the flag.
// Passeserr = makeCmd("def.com", "").Invoke("--url", "sup", "--alt-url", "hup").Run()require.NoError(t, err)require.Equal(t, serpent.String("hup"), got)// Failserr = makeCmd("def.com", "").Invoke("--alt-url", "hup", "--url", "sup").Run()require.NoError(t, err)require.Equal(t, serpent.String("hup"), got)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.
I don't think alphabetical order is going to be intuitive. The current behaviour of "last one wins" makes more sense to me.
Alternatively, we could say that specifying a non-default value for both is an error? That would remove any ambiguity, but would be a breaking change.
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.
@Emyrk it's typical behavior for CLI programs to use the last value of any given flag, for example:
~/Work/Code/serpent mafredri/fix-shared-value-overwrite*❯ git log -1 --format='%h'e053fda~/Work/Code/serpent mafredri/fix-shared-value-overwrite*❯ git log -1 --format='%h' --format='%H'e053fdac0b0dd01a0e6278dfbd703bbf7486a840So I would say the failing test-case is working as intended, just the assertion that should change.
I added some more test-cases to cover this and also the env/flag priority one in457cbbe.
@johnstcn In this PR we already handle the case where flags specify multiple defaults, and that's OK as long as:
- Either only one specifies a default
- Either one or more specify the same exact default
- If two different defaults are detected, that's an error
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.
This made me curious. In the case of envs, it seems to be "first wins". I tested with the following test-cases.
// Both envs are given, first wins.inv=makeCmd("def.com","").Invoke()inv.Environ.Set("URL","sup")inv.Environ.Set("ALT_URL","hup")err=inv.Run()require.NoError(t,err)require.Equal(t,"sup",got)// Both envs are given, first wins #2.inv=makeCmd("def.com","").Invoke()inv.Environ.Set("ALT_URL","hup")inv.Environ.Set("URL","sup")err=inv.Run()require.NoError(t,err)require.Equal(t,"sup",got)
I'm not sure if we need to do anything about that, but just keep in mind that this behavior has not changed in this PR.
Uh oh!
There was an error while loading.Please reload this page.
| if opt.Default == "" { | ||
| continue | ||
| } | ||
| if optWithDefault != nil && optWithDefault.Default != opt.Default { | ||
| merr = multierror.Append( | ||
| merr, | ||
| xerrors.Errorf( | ||
| "parse %q: multiple defaults set for the same value: %q and %q (%q)", | ||
| opt.Name, opt.Default, optWithDefault.Default, optWithDefault.Name, | ||
| ), | ||
| ) | ||
| continue | ||
| } | ||
| optWithDefault = opt |
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.
suggestion: this also looks like it could be extracted to its own function and tested independently
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.
I don't see a benefit currently as this is pretty much "the meat" of this function and we don't need to call it elsewhere.
| err = makeCmd("def.com", "").Invoke("--alt-url", "hup").Run() | ||
| require.NoError(t, err) | ||
| require.Equal(t, "hup", got) |
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.
I don't think alphabetical order is going to be intuitive. The current behaviour of "last one wins" makes more sense to me.
Alternatively, we could say that specifying a non-default value for both is an error? That would remove any ambiguity, but would be a breaking change.
Emyrk left a comment
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.
I updated my go mod to this branch, and that failing you mentioned test is passing locally.
| if a.ValueSource != b.ValueSource { | ||
| for _, vs := range valueSourcePriority { | ||
| if a.ValueSource == vs { | ||
| return -1 | ||
| } | ||
| if b.ValueSource == vs { | ||
| return 1 | ||
| } | ||
| } | ||
| } |
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.
This is totally fine, and an optimized solution. My morning brain just took a second to understand it.
Can you just throw a comment like,// Sorts by value source then a default value being set
I like using subtraction like this for compare functions.
if a.ValueSource != b.ValueSource { return slices.Index(valueSourcePriority, a.ValueSource) - slices.Index(valueSourcePriority, b.ValueSource)}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.
That's clean, thanks! 👍🏻
johnstcn left a comment
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.
Thanks for doing this Mathias!
ammario commentedNov 13, 2024
My instinct would be to error if the value is the same as the semantics around priority are not immediately obvious and perhaps better for clients to decide with their own logic. It seems like you have a lot of eyes here already so feel free to merge at will. |
mafredri commentedNov 13, 2024
Yeah, I was a bit torn myself between the two. In the end settled on this approach because it makes maintenance of deprecated flags a lot easier, especially if all you're doing is renaming or reorganizing. If we enforce the use of two different unique values, then there must always exist logic to check both or merge them at an appropriate time, and that seems like a recipe for bugs. |
Emyrk commentedNov 13, 2024
Agreed. I was sold when@johnstcn mentioned flag order already overrides. And it is the existing behavior. |
Uh oh!
There was an error while loading.Please reload this page.
While working on tests forcoder/internal#209 I noticed we don't properly handle shared values between options.
Perhaps this isn't the intended use-case, but it's definitely something that happens in the real world. Consider this example:
See how both options reference
c.Notifications.SMTP.From? This is still OK, but if the user adds aDefaultto either (or both) of these, the behavior will be very unexpected.This PR fixes this in a way that handles this use-case. A simpler approach would be to return an error if two flags reference the same value, but I feel that would be restrictive and go against common Go patterns with regard to flag parsing (i.e. sharing the same var).