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

Jump to bottom

Configure GenericFlag's Destination type as struct not pointer #1442

Merged
merged 6 commits into from Oct 6, 2022
Merged

Configure GenericFlag's Destination type as struct not pointer #1442

merged 6 commits into from Oct 6, 2022

Conversation

nkuba
Copy link
Contributor

@nkuba nkuba commented Jul 26, 2022

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

@nkuba nkuba requested a review from a team as a code owner July 26, 2022 14:23
@nkuba nkuba mentioned this pull request Jul 26, 2022
Closed
3 tasks
@joeycumines
Copy link
Contributor

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
Copy link
Contributor Author

nkuba commented Jul 28, 2022

(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
Copy link
Contributor

@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
dearchap previously approved these changes Aug 1, 2022
View reviewed changes
@dearchap
Copy link
Contributor

@nkuba Can you rebase your changes with latest ?

@meatballhat meatballhat added the status/conflicts contains merge conflicts label Sep 10, 2022
dearchap
dearchap reviewed Sep 10, 2022
View reviewed changes
flag-spec.yaml Outdated
@@ -12,35 +12,41 @@ flag_types:
uint: {}

string:
destination_pointer: true
Copy link
Contributor

@dearchap dearchap Sep 10, 2022 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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 dearchap added status/waiting-for-response Waiting for response from original requester area/v2 relates to / is being considered for v2 labels Sep 26, 2022
nkuba and others added 5 commits October 5, 2022 22:04
@nkuba @dearchap
Add DestinationPointer for flags generator
891ffb0 
In this commit I added a DestinationPointer variable that should be set
if the `Destination` should be configured as a pointer for a specific
flag type. It is expected that Generic type which is an interface will
not be a pointer but a struct. Before this change the code compilation
was failing with `type *Generic is pointer to interface, not interface`.

See urfave#1441
@nkuba @dearchap
Set destination in GenericFlag apply function
a2541e0 
The function was missing destination configuration.
@nkuba @dearchap
Add unit test for GenericFlag Destination parsing
c472192 
The test checks if Destination provided in GenericFlag is being set as
expected.
@dearchap
After rebase
d724a63
@dearchap
Fix compile
a9c758e
@dearchap dearchap dismissed stale reviews from ghost and themself via a9c758e October 6, 2022 02:16
@dearchap dearchap force-pushed the generit-flag-destination-pointer branch from 341be3b to a9c758e Compare October 6, 2022 02:16
dearchap
dearchap approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@dearchap dearchap left a comment

Choose a reason for hiding this comment

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

Fixed merge issues

@dearchap dearchap merged commit 8f469ab into urfave:main Oct 6, 2022
@meatballhat meatballhat mentioned this pull request Oct 9, 2022
Closed
6 tasks
another-rex added a commit to google/osv.dev that referenced this pull request Oct 13, 2022
@renovate-bot @another-rex
Update tools (#768)
ff6d8a7 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/g-rath/osv-detector](https://togithub.com/g-rath/osv-detector)
| require | minor | `v0.7.2` -> `v0.8.0` |
| [github.com/urfave/cli/v2](https://togithub.com/urfave/cli) | require
| minor | `v2.17.1` -> `v2.19.2` |
| [golang.org/x/crypto](https://togithub.com/golang/crypto) | require |
digest | `4161e89` -> `56aed06` |
| [golang.org/x/exp](https://togithub.com/golang/exp) | require | digest
| `b9f4876` -> `4de253d` |

---

### Release Notes

<details>
<summary>g-rath/osv-detector</summary>

###
[`v0.8.0`](https://togithub.com/G-Rath/osv-detector/releases/tag/v0.8.0)

[Compare
Source](https://togithub.com/g-rath/osv-detector/compare/v0.7.2...v0.8.0)

#### What's Changed

- support parsing `poetry.lock`, for Python
([G-Rath/osv-detector#156)
- support parsing `pubspec.lock`, for Dart
([G-Rath/osv-detector#159)

**Full Changelog**:
G-Rath/osv-detector@v0.7.2...v0.8.0

</details>

<details>
<summary>urfave/cli</summary>

### [`v2.19.2`](https://togithub.com/urfave/cli/releases/tag/v2.19.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.19.1...v2.19.2)

#### What's Changed

- fix: stop automatic sorting for --help by
[@&#8203;FGYFFFF](https://togithub.com/FGYFFFF) in
[urfave/cli#1430

#### New Contributors

- [@&#8203;FGYFFFF](https://togithub.com/FGYFFFF) made their first
contribution in
[urfave/cli#1430

**Full Changelog**:
urfave/cli@v2.19.1...v2.19.2

### [`v2.19.1`](https://togithub.com/urfave/cli/releases/tag/v2.19.1)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.19.0...v2.19.1)

#### What's Changed

- Fix:(issue\_1500). Fix slice flag value duplication issue by
[@&#8203;dearchap](https://togithub.com/dearchap) in
[urfave/cli#1502

**Full Changelog**:
urfave/cli@v2.19.0...v2.19.1

### [`v2.19.0`](https://togithub.com/urfave/cli/releases/tag/v2.19.0)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.2...v2.19.0)

#### What's Changed

- Fix:(issue\_1505) Fix flag alignment in help by
[@&#8203;dearchap](https://togithub.com/dearchap) in
[urfave/cli#1506

**Full Changelog**:
urfave/cli@v2.18.2...v2.19.0

### [`v2.18.2`](https://togithub.com/urfave/cli/releases/tag/v2.18.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.1...v2.18.2)

#### What's Changed

- Configure GenericFlag's Destination type as struct not pointer by
[@&#8203;nkuba](https://togithub.com/nkuba) in
[urfave/cli#1442

#### New Contributors

- [@&#8203;nkuba](https://togithub.com/nkuba) made their first
contribution in
[urfave/cli#1442

**Full Changelog**:
urfave/cli@v2.18.1...v2.18.2

### [`v2.18.1`](https://togithub.com/urfave/cli/releases/tag/v2.18.1)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.18.0...v2.18.1)

#### What's Changed

- Ensure "generate" step runs in CI prior to diff check by
[@&#8203;meatballhat](https://togithub.com/meatballhat) in
[urfave/cli#1504

**Full Changelog**:
urfave/cli@v2.18.0...v2.18.1

### [`v2.18.0`](https://togithub.com/urfave/cli/releases/tag/v2.18.0)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.17.2...v2.18.0)

#### What's Changed

- Call FlagStringer in String() method of slice flags by
[@&#8203;fjl](https://togithub.com/fjl) in
[urfave/cli#1508

#### New Contributors

- [@&#8203;fjl](https://togithub.com/fjl) made their first contribution
in
[urfave/cli#1508

**Full Changelog**:
urfave/cli@v2.17.2...v2.18.0

### [`v2.17.2`](https://togithub.com/urfave/cli/releases/tag/v2.17.2)

[Compare
Source](https://togithub.com/urfave/cli/compare/v2.17.1...v2.17.2)

#### What's Changed

- Remove nonexistent phony targets by
[@&#8203;meatballhat](https://togithub.com/meatballhat) in
[urfave/cli#1503
- wrap: Avoid trailing whitespace for empty lines by
[@&#8203;abitrolly](https://togithub.com/abitrolly) in
[urfave/cli#1513

#### New Contributors

- [@&#8203;abitrolly](https://togithub.com/abitrolly) made their first
contribution in
[urfave/cli#1513

**Full Changelog**:
urfave/cli@v2.17.1...v2.17.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on monday" in timezone
Australia/Sydney, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/google/osv.dev).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMjIuMyIsInVwZGF0ZWRJblZlciI6IjMyLjIzNC4yIn0=-->

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dearchap dearchap dearchap approved these changes

Assignees
No one assigned
Labels
area/v2 relates to / is being considered for v2 status/conflicts contains merge conflicts status/waiting-for-response Waiting for response from original requester
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GenericFlag doesn't work with Destination: type *Generic is pointer to interface, not interface
4 participants
@nkuba @joeycumines @dearchap @meatballhat