cli
cli copied to clipboard
Configure GenericFlag's Destination type as struct not pointer
What type of PR is this?
- bug
What this PR does / why we need it:
This PR implements support for Destination
property in GenericFlag
. Previously setting a Destination was causing code compilation failures described in https://github.com/urfave/cli/issues/1441. The type of Destination
property was removed with the pointer so it works correctly with a Generic
interface now.
Which issue(s) this PR fixes:
Fixes https://github.com/urfave/cli/issues/1441
Special notes for your reviewer:
I had problems with running the code generator on my local machine. Please run code generation and verify its result.
Testing
I added a unit test covering destination setting for GenericFlag.
Release Notes
Implemented Destination setting for GenericFlag (fixes #1441).
This is technically a breaking change.
From a pragmatic standpoint, it might still be sensible to merge it into v2.
I'd say that it depends how long the Destination
field in question has existed for.
(I'm assuming but haven't actually checked that it is possible to get it working, currently, and that it's just a dodgy API)
(I'm assuming but haven't actually checked that it is possible to get it working, currently, and that it's just a dodgy API)
It would be helpful if you could share the code example how to do it with the current version. I've been struggling to do it with no success unfortunately.
@nkuba actually... it seems like you could use the Value
field as the Destination
. The semantics of the flag.Value
interface doesn't make implementing the desired behavior particularly easy.
(I'm assuming but haven't actually checked that it is possible to get it working, currently, and that it's just a dodgy API)
It would be helpful if you could share the code example how to do it with the current version. I've been struggling to do it with no success unfortunately.
Mmm I should have phrased that "working with the current (dodgy) API", because it does appear to be missing the actual behavior necessary to make the Destination
field do anything useful. This behavior appears to have been partially corrected in this PR, which I'll try and explain in a bit...
https://github.com/urfave/cli/blob/341be3b6b2cbf956d181e57212f4eab81446202b/flag_generic.go#L66
I'm guessing this is more fallout from when the Destination
field was added, but not actually implemented, for many of the flag types. Since it didn't actually work, in the old implementation, the field type change in this PR looks good to me, despite being a breaking change, in the strictest sense. I suspect the best fix would be to remove that Destination
field entirely, however.
It appears that with the current state of this PR, Destination
won't play nice with either default values (provided via the Value
field), or configuration provided via env vars.
https://github.com/urfave/cli/blob/main/docs/v2/manual.md#precedence
If the Destination
field is kept:
I'd like it to avoid mutating the Value
field, and have comparable behavior as the changes I made to the slice flag implementations in https://github.com/urfave/cli/pull/1409. Those cases weren't working with interfaces, however, so "cloning" (and updating one value from another, for that matter) was far more feasible.
I believe the ideal general flow of the Apply
method would be:
- Apply any default, by updating
Destination
with the value ofValue
, if both are non-nil - Resolve a "target"
flag.Value
, which will be one ofDestination
(if non-nil), a copy ofValue
(if non-nil) or a default (invalid in this case, I guess) - If the env var is set, set the value into target resolved in 2. (don't update the
Value
field), and mark thecli.Flag
as set (theHasBeenSet
field) - For each of the flag names,
set.Var(target, name, f.Usage)
@nkuba Can you rebase your changes with latest ?