Configure GenericFlag's Destination type as struct not pointer

[nkuba]

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 #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 #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).

[joeycumines]

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)

[nkuba]

(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.

[joeycumines]

@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...

cli/flag_generic.go

Line 66 in 341be3b

set.Var(f.Destination, name, f.Usage)

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 #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:

1. Apply any default, by updating Destination with the value of Value, if both are non-nil
2. Resolve a "target" flag.Value, which will be one of Destination (if non-nil), a copy of Value (if non-nil) or a default (invalid in this case, I guess)
3. If the env var is set, set the value into target resolved in 2. (don't update the Value field), and mark the cli.Flag as set (the HasBeenSet field)
4. For each of the flag names, set.Var(target, name, f.Usage)

[dearchap]

@nkuba Can you rebase your changes with latest ?

[dearchap]

Instead of defining destination_pointer to true for everything except Generic have a field called no_destination_pointer and set that to true only for Generic. That way all these changes go away and any new type by default will have the pointer unless explicitly specified

[dearchap]

@nkuba I've made PR 1488 which cleans up the destination pointer for Generic so that you dont have to worry about fixing conflicts in your branch as it is very tedious and time consuming(I tried, trust me). Once that is approved you can revert your changes for flag spec and keep just the test code. Thank you so much

[dearchap]

Fixed merge issues