8000 Adding support for default from the env by meatballhat · Pull Request #108 · urfave/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 4 commits into from
Aug 2, 2014

Conversation

meatballhat
Copy link
Member

No description provided.

@codegangsta
Copy link
Contributor

Nice test coverage. Looking at this now

@codegangsta
Copy link
Contributor

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
@meatballhat
Copy link
Member Author

@codegangsta thanks for the feedback! Merge conflicts resolved. How best to ask for community feedback?

@AudriusButkevicius
Copy link
Contributor

I definitely think this is a good idea.

@kytrinyx
Copy link
Contributor

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.

@codegangsta
Copy link
Contributor

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

On Jul 16, 2014, at 6:37 AM, Katrina Owen notifications@github.com wrote:

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.


Reply to this email directly or view it on GitHub.

@jszwedko
Copy link
Contributor

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 gofix rule that users could use.

8000

@jszwedko
Copy link
Contributor

Working on a gofix shortly

@jszwedko
Copy link
Contributor

Never mind, I didn't realize that the go fix tool doesn't expose an API (presumably only to be used, then, for standard library changes).

@meatballhat
Copy link
Member Author

bump 😺

@meatballhat
Copy link
Member Author

And bumping again 😺

Anything I should change?

@codegangsta
Copy link
Contributor

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

@meatballhat
Copy link
Member Author

@codegangsta no worries! just giving the plate a spin

@codegangsta
Copy link
Contributor

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.

codegangsta added a commit that referenced this pull request Aug 2, 2014
Adding support for default from the env
@codegangsta codegangsta merged commit 565493f into urfave:master Aug 2, 2014
@codegangsta
Copy link
Contributor

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

bjeanes added a commit to bjeanes/go-lifx that referenced this pull request Aug 6, 2014
urfave/cli#108 changed the struct shape so the literal struct invocation
broke. Now using explicit field names instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0