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

v2 feature: Add Destination for StringSliceFlag #1075

Closed
3 tasks done
davidsbond opened this issue Feb 25, 2020 · 12 comments · Fixed by #1078
Closed
3 tasks done

v2 feature: Add Destination for StringSliceFlag #1075

davidsbond opened this issue Feb 25, 2020 · 12 comments · Fixed by #1078
Assignees
Labels
area/v2 relates to / is being considered for v2 status/claimed someone has claimed this issue

Comments

@davidsbond
Copy link
Contributor

davidsbond commented Feb 25, 2020

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this feature? Here's the Github guide about searching.

What problem does this solve?

  • My team want to start using the Destination values on flags so we can have our flag values strongly typed rather than stringly typed (i.e calling c.String("flag") etc.). I noticed that the StringSliceFlag does not have a Destination field like other flags do

Solution description

  • Solution would be to add a Destination field to StringSliceFlag that works the same as all other Destination flag fields do.
@davidsbond davidsbond added area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this labels Feb 25, 2020
@saschagrunert
Copy link
Member

I think this might be a valuable addition :)

@coilysiren coilysiren added help wanted please help if you can! status/confirmed confirmed to be valid, but work has yet to start and removed status/triage maintainers still need to look into this labels Feb 26, 2020
@coilysiren
Copy link
Member

See also => #603 🙂

@tboerger
Copy link

Did you perform a search about this feature? Here's the Github guide about searching.

Not really searched for that, right? Till yesterday that issue have been even open and have been closed for being a duplicate of this issue here... https://github.com/urfave/cli/issues?q=is%3Aissue+StringSliceFlag+Destination+is%3Aclosed

@coilysiren
Copy link
Member

@tboerger sorry you're having a hard time with the duplication thing! Is there something specific that you want here?

@davidsbond
Copy link
Contributor Author

@tboerger Sorry, obviously I didn't search very hard when I raised this. Agree mine should've been closed and yours left open. Happy for either issue to be used as long as it results in the feature being implemented.

@davidsbond
Copy link
Contributor Author

@lynncyrin I have some free tech time at work tomorrow so I can look into implementing this

@tboerger
Copy link

It's more about the communication than marking something duplicate. The initial issue has been marked as claimed, which sounds like somebody would work on that, after some time nothing happened and I actively asked if I can help somehow as this lib is massively used in projects I'm involved and I haven't received any response. Just with the closing as duplicate comment I have seen that the claimed label have been removed.

I would just expect a tiny comment if somebody offers help. They issue is already nearly three years old.

@davidsbond
Copy link
Contributor Author

davidsbond commented Feb 27, 2020

@tboerger I see what you mean. Well I'll be looking to raise a PR to add this functionality sometime tomorrow, unless you'd like to do so yourself.

I also notice there are a few more flags missing the Destination field. It might be worth aggregating all those and doing a single issue for missing Destination fields.

@tboerger
Copy link

Go ahead, I would be happy to help testing.

@coilysiren coilysiren added status/claimed someone has claimed this issue and removed help wanted please help if you can! status/confirmed confirmed to be valid, but work has yet to start labels Feb 27, 2020
@coilysiren
Copy link
Member

Got it 👍 it wasn't previously clear to me that there's so much social complexity here 🙏

@davidsbond I just assigned this to you 👍

@davidsbond
Copy link
Contributor Author

@lynncyrin @tboerger I think my PR is ready for review. Appears to work in my testing

@coilysiren
Copy link
Member

coilysiren commented Feb 29, 2020

This should go out in version 2.2.0 whenever someone feels so compelled to make another release

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 status/claimed someone has claimed this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants