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

add support for an optional --version flag #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

costela
Copy link
Contributor

@costela costela commented Jan 29, 2019

This PR adds support for a simple--version flag. It is only enabled if a value is provided to the new Conf.VersionString setting.

The current implementation is a bit less customizable than the help settings (no VersionMessage or VersionDescription).

This also expands testing to check command output, with a - maybe overkill? - panic/recover combo. Please let me know if you think this should be handled some other way.

@costela
Copy link
Contributor Author

costela commented Jan 29, 2019

Replaced strings.Builder usage with bytes.Buffer, which should be available in go < 1.10.

Copy link
Owner

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

I didn't know --version emits exit code 2, tbh.

help.go Outdated Show resolved Hide resolved
@costela
Copy link
Contributor Author

costela commented Jan 29, 2019

I didn't know --version emits exit code 2, tbh.

Yeah, I was wondering why --help does it, but didn't wanna change it "just because". Maybe there's someone out there depending on this behavior.

OTOH, maybe --version doesn't have to imitate it...

Thoughts?

@stevenroose
Copy link
Owner

I think I found that exit code 2 was for "misuse of the command line arguments", so basically when the help is printed not with --help but with a parsing failure or an unknown flag or so.

I guess I was just lazy and had help always exit 2. I think exiting "OK" for version is better.

@costela
Copy link
Contributor Author

costela commented Jan 30, 2019

Fair enough.
New version does exit(0) and is a bit more specific during testing, avoiding shadowing actual panics in the code. (note that we now only check if we exit, not the return code)

@stevenroose
Copy link
Owner

Yeah ok, this might make sense.

@stevenroose
Copy link
Owner

stevenroose commented Jan 30, 2019

Perhaps still not entirely convinced why gonfig should do this, though.

I mean a program could have a

Version bool `short:"b"`

and then just in the first line of the program

if conf.Version {
    fmt.Println("v0.1.3")
    os.Exit(0)
}

That's like 5 lines of code that you're trying to reduce to one, kinda.

@costela
Copy link
Contributor Author

costela commented Jan 30, 2019

True, just that this is boilerplate that I'd bet is basically everywhere. Just felt like something gonfig might make even easier and less "boilerplaty' with minimal effort (considering the biggest chunk of added code is testing that arguably should already be there for covering the exit-behavior of --help anyway).

But of course, this is a judgment call. No problem if you think this is just not the place for it. Feel free to close the PR! 👍

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.

2 participants