-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Adding support for default from the env #108
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
Conversation
Nice test coverage. Looking at this now |
My only issue with this is that we will be breaking others implementations that have used the alternative syntax for struct declarations. At this point I'm okay with doing this, declaring structs that way should no longer be recommended and it is an easy enough fix. I would like to know what others think of pushing a breaking change like this? |
Conflicts: README.md app_test.go cli_test.go
@codegangsta thanks for the feedback! Merge conflicts resolved. How best to ask for community feedback? |
I definitely think this is a good idea. |
I'm personally OK with this, but I am a bit uncomfortable with the idea of a breaking change happening with a patch or minor release. |
Yeah. Although the Go 1 compatibility promise only does so for tagged struct literals http://golang.org/doc/go1compat This makes me feel a little better my only uneasiness at this point is the fact that the readme in master uses untagged struct literals. I think it may be a necessary pain to go through. :/ Sent from my iPhone
|
Yeah, the standard library has made breaking changes for untagged struct literals in the past (e.g. https://code.google.com/p/go/issues/detail?id=4499) so I feel that this is OK. We could write/include a |
Working on a |
Never mind, I didn't realize that the |
bump 😺 |
And bumping again 😺 Anything I should change? |
hey @meatballhat, I've been super busy and off/on vacation. I intend to do a sweep of all the cli issues and PRs very soon. Apologies for being unresponsive |
@codegangsta no worries! just giving the plate a spin |
Awesome addition! I'm going to land this and make another release of CLI. @meatballhat thanks for making the switch to better struct literals, this is going to be a much better API moving forward. |
Adding support for default from the env
btw @meatballhat I'm adding you as a collaborator. Feel free to look over the contribution guidelines when you get the chance. https://github.com/codegangsta/cli#contribution-guidelines Thanks for all your contributions :) |
urfave/cli#108 changed the struct shape so the literal struct invocation broke. Now using explicit field names instead.
No description provided.