search

davecheney / httpstat

It's like curl -v, with colours.
MIT License
6.95k stars 382 forks source link

GNU command line flag handling using go-flags #87

Closed theckman closed 7 years ago

theckman commented 7 years ago

While reviewing the PR to add version output to httpstat, I thought it would be nice to have parity with curl's flag. In addition to that, with us aiming for a 1.0 release, it'd be nice to have assertions around the command line argument parsing.

This changeset changes the command line flag parsing from the standard library to that of go-flags. The way go-flags is consumed here is a bit complicated, but it's so that we can abstract functionality away and properly unit test all command line flag parsing.

This also fixes a bug in the error handling around the output file creation. Previously, if the file couldn't be created it may have used the wrong filename in the error message.

theckman commented 7 years ago

@davecheney I understand that this functionality may not be desired, but I thought it may be worth raising this as we prep for v1.0.

theckman commented 7 years ago

Here is the example output:

Usage:
  httpstat [OPTIONS] <url>

Application Options:
  -d, --data=        the body of a POST or PUT request
  -H, --header=      HTTP Header(s) to set. Can be used multiple times -H 'Accept:...' -H 'Range...'
  -I, --head         don't read the body of the request
  -k, --insecure     allow insecure TLS connections
  -L, --location     follow 30x redirects
  -o, --output=      output file for body
  -O, --remote-name  Save body as remote filename
  -V, --version      print version to stdout and exit
  -X, --request=     HTTP method to use (default: GET)

Help Options:
  -h, --help         Show this help message
theckman commented 7 years ago

Orrrrrr nevermind. Just ran in to #28 and saw that it was closed because we didn't want to track another dependency.

Sounds like I may have just wasted a few hours on this. If you don't want another dependency feel free to close this PR. There is a bugfix in here that I'll need to provide in a new PR, so give me a shout either way.

davecheney commented 7 years ago

I'm sorry you wasted your time. That is why the policy of this project is to raise an issue before coding.

On 28 Sep. 2016, at 17:51, Tim Heckman notifications@github.com wrote:

Orrrrrr nevermind. Just ran in to #28 and saw that it was closed because we didn't want to track another dependency.

Sounds like I may have just wasted a few hours on this. If you don't want another dependency feel free to close this PR. There is a bugfix in here that I'll need to provide in a new PR, so give me a shout either way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

theckman commented 7 years ago

@davecheney if I wasted my inspiration on opening tickets I'd never get any code done. 😂

No biggie.

theckman commented 7 years ago

@davecheney I'll close this PR and maintain the fork with GNU-style flags. Let me know if you want an issue for the filename thing.

davecheney commented 7 years ago

Thanks. I'm not looking to add another dependency, I'm mad that we have to vendor http2 just to turn on a setting that controls the copy of http2 that is vendored into the standard library.

If you want, two things:

  1. please open an issue for the filename problem you found, or just peal off the part of your change and send it as a PR.
  2. I'm pretty confident that you could change the usage message we have to give us almost the same usage message as your PR provided. If you want to do that, please go right ahead.

On Wed, Sep 28, 2016 at 6:49 PM, Tim Heckman notifications@github.com wrote:

Closed #87 https://github.com/davecheney/httpstat/pull/87.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/davecheney/httpstat/pull/87#event-805112105, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcA_r3cH_big-9hiLbjs14gwXkxkl0ks5quioNgaJpZM4KIgu6 .

theckman commented 7 years ago

I guess I've become less mad about dependencies now that it's not completely the wild-west anymore. I think it's reasonable to have a handful of dependencies to have a fully-featured utility. I'm mad that they thought the flag package was good-enough to ship in the stdlib. 😄

My goals here were simply:

The usage message was unimportant to me. The go-flags package just provided it and I thought I'd include the output in case anyone had any objections. I don't personally have an interest in changing it otherwise.

I'll open a bug report around the error message.

davecheney commented 7 years ago

The usage message is very important to me. It's what every user see when they type the name of the program.

Parity with curl is not a goal of this tool, a version of httpstat.py that was written in Go was the goal. We've pretty much ticket that goal now and looking good to put a bow around it on Sunday.

On Wed, Sep 28, 2016 at 7:00 PM, Tim Heckman notifications@github.com wrote:

I guess I've become less mad about dependencies now that it's not completely the wild-west anymore. I think it's reasonable to have a handful of dependencies to have a fully-featured utility. I'm mad that they thought the flag package was good-enough to ship in the stdlib. 😄

My goals here were simply:

  • reasonable CLi flags, preferring parity with curl
  • unit testing of the flags parsing to ensure options are properly enabled/disabled

The usage message was unimportant to me. The go-flags package just provided it and I thought I'd include the output in case anyone had any objections. I don't personally have an interest in changing it otherwise.

I'll open a bug report around the error message.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/davecheney/httpstat/pull/87#issuecomment-250111319, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcA_qtFfo2cC0KdvXxKyAwy9SRSwJIks5quiyZgaJpZM4KIgu6 .