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

Fix flag shorthand conflict. #134

Merged
merged 1 commit into from
Sep 13, 2018
Merged

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Sep 13, 2018

Both the "verbose" flag in the cli root and the "version" flag in the
read command use the "v" shorthand, which causes a panic on read. This
patch drops the shorthand for the "version" flag.

@nickatsegment

@nickatsegment
Copy link
Contributor

@jmcarp Thanks for this! Panics: they're bad.

My gut feeling is that we should drop the short form of verbose because I think it's newest, and isn't yet in a release. Will take a closer look tomorrow.

@jmcarp
Copy link
Contributor Author

jmcarp commented Sep 13, 2018

I didn't realize the --verbose flag was newer. Just amended to drop the shorthand for --verbose instead of --version to avoid a breaking change.

Both the "verbose" flag in the cli root and the "version" flag in the
read command use the "v" shorthand, which causes a panic on read. This
patch drops the shorthand for the "verbose" flag.
Copy link
Contributor

@systemizer systemizer left a comment

Choose a reason for hiding this comment

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

Ah good catch. LGTM

@nickatsegment nickatsegment merged commit e61fff1 into segmentio:master Sep 13, 2018
@nickatsegment
Copy link
Contributor

Merged! Thanks again @jmcarp; you're killing it.

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.

3 participants