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

Show vendor plugin help when no command is given #449

Merged
merged 1 commit into from Nov 12, 2015

Conversation

Projects
None yet
2 participants
@kalafut
Contributor

kalafut commented Nov 11, 2015

I don't have much familiarity with gb internals, but I use it a lot and saw the "Help Wanted" sign. This PR may not be exactly what you're looking for. I couldn't come up with a good reason why args < 1 should cause an error at the gb level... why not let the plugins worry about that? Also, the various exit codes in gb-vendor were causing ugly "FATAL: ..." at the end of the help, so I pulled them.

Fixes #446

@davecheney

This comment has been minimized.

Contributor

davecheney commented Nov 11, 2015

Thanks for this, but I don't think this is quiet right. Looking at cmd/gb it does something like this

        args := os.Args
        if len(args) < 2 || args[1] == "-h" {
                fs.Usage()
                os.Exit(1)
        }

If you adapt this to gb-vendor and use os.Exit(0) with a comment explaining what this does to the caller. This should be all that is needed.

@kalafut

This comment has been minimized.

Contributor

kalafut commented Nov 11, 2015

Hi Dave,

I'll revisit this, though I do have a couple of questions:

  1. What do you want fs.Usage() to do in general with respect to error codes? In your code snippet, fs.Usage() (set to usage() in cmd/gb/help.go) prints the usage and then exits with error code 2. The os.Exit(1) is never hit. This also results in:

    gb help    # (prints help, exit code 0, which seems correct BTW)
    gb         # (prints identical help, exit code 2)
    

    Within gb-vendor there is an unused usage() function, and then fs.Usage defined in init(). Looks like this could be consolidated but I'd like to know your goal for the help system.

  2. I wasn't sure if your last sentence indicated that cmd/gb/main.go didn't need to change. I think it has to, but I wanted to double check..

Thanks,
Jim

@davecheney

This comment has been minimized.

Contributor

davecheney commented Nov 11, 2015

]cmd/gb/help.go) prints the usage and then exits with error code 2.
The os.Exit(1) is never hit. This also results in:

Oh, I didn't realise that.

gb help # (prints help, exit code 0, which seems correct BTW)
gb # (prints identical help, exit code 2)

Within gb-vendor there is an unused usage() function, and then
fs.Usage defined in init(). Looks like this could be consolidated but I'd
like to know your goal for the help system.

I don't have much of a goal for the help system, it's pretty minimal at
the moment, idiosyncratic, and slightly broken, sadly it's not top of the
list atm.

I wasn't sure if your last sentence indicated that cmd/gb/main.go
didn't need to change. I think it has to, but I wanted to double check..

It's fine for gb to exit with error 2, that is a sign to the caller they
got something wrong (possibly scripted), gb-vendor isn't intended to be
called directly, so if we need to make the exit codes change to suppress a
message from gb (which thinks it is calling a plugin), that is fine.

Thanks,
Jim


Reply to this email directly or view it on GitHub
#449 (comment).

@kalafut kalafut force-pushed the kalafut:master branch from 7c66501 to 5d8c4f6 Nov 12, 2015

@kalafut

This comment has been minimized.

Contributor

kalafut commented Nov 12, 2015

I thought about it some more and since the custom fs.Usage() function is called by FlagSet when parsing bad input, keeping the os.Exit() there makes sense.

I updated the PR to just call printUsage() directly in case of default help and then exit 0. This provides clean behavior for gb vendor without any error text.

@davecheney

This comment has been minimized.

Contributor

davecheney commented Nov 12, 2015

LGTM. Thank you very much.

Don't worry about the travis failure, we're just getting it going and haven't cut over yet.

davecheney added a commit that referenced this pull request Nov 12, 2015

Merge pull request #449 from kalafut/master
Show vendor plugin help when no command is given

@davecheney davecheney merged commit 93c54d9 into constabulary:master Nov 12, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/wercker Wercker build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment