davecheney / httpstat

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

Use spf13/pflag package for flags #28

Closed n10v closed 7 years ago

n10v commented 7 years ago

It's also helpful to implement multiple -H flags, because without StringSliceVarP method I have no idea how to implement it

n10v commented 7 years ago

We can define our own usage function for flags Which user message do you want?

moorereason commented 7 years ago

@bogem, can you incorporate the diff below?

@davecheney, I'm no expert on flag packages, but github.com/spf13/pflag is the one I have the most experience with. It's lean. I tried using github.com/alecthomas/kingpin, but the size of the resulting binary was 40% larger (2MB+) than pflag's.

With the following patch, the help output looks like this:

Usage: httpstat [flags] <URL>

Flags:
  -d, --data body        the body of a POST or PUT request
  -I, --head             don't read body of request
  -h, --help             show usage information
  -L, --location         follow 30x redirects
  -X, --request method   HTTP method to use (default "GET")
diff --git a/main.go b/main.go
index 02a1dc7..dd0664b 100644
--- a/main.go
+++ b/main.go
@@ -58,18 +58,18 @@ var (
        postBody        string
        followRedirects bool
        onlyHeader      bool
-
-       usage = fmt.Sprintf("usage: %s [flags] URL", os.Args[0])
+       help            bool
 )

 func init() {
-       pflag.StringVarP(&httpMethod, "request", "X", "GET", "HTTP method to use")
-       pflag.StringVarP(&postBody, "data", "d", "", "the body of a POST or PUT request")
+       pflag.StringVarP(&httpMethod, "request", "X", "GET", "HTTP `method` to use")
+       pflag.StringVarP(&postBody, "data", "d", "", "the `body` of a POST or PUT request")
        pflag.BoolVarP(&followRedirects, "location", "L", false, "follow 30x redirects")
        pflag.BoolVarP(&onlyHeader, "head", "I", false, "don't read body of request")
+       pflag.BoolVarP(&help, "help", "h", false, "show usage information")

        pflag.Usage = func() {
-               fmt.Fprintln(os.Stderr, usage)
+               fmt.Fprintf(os.Stderr, "Usage: httpstat [flags] <URL>\n\nFlags:\n")
                pflag.PrintDefaults()
        }
 }
@@ -78,9 +78,12 @@ func main() {
        pflag.Parse()

        args := pflag.Args()
-       if len(args) != 1 {
+       if len(args) != 1 || help {
                pflag.Usage()
-               os.Exit(2)
+               if !help {
+                       os.Exit(2)
+               }
+               os.Exit(0)
        }

        url, err := url.Parse(args[0])
n10v commented 7 years ago

@moorereason yes, of course But I am not sure, what the usage of flag data is really clear. I think, we should write another usage or use the curl's one:

-d, --data DATA HTTP POST data

moorereason commented 7 years ago

The only caveat with pflag usage printing is that the flag argument placeholder must be surrounded by back-quotes in the flag description. So, you can't use "data" in the description and get "DATA" in the example.

n10v commented 7 years ago

@moorereason yes, I know it. It was just a copy-paste form curl usage. Then we can do so: -d, --data data HTTP POST data But looks not nice. Ok, the way you offer is better, I'll do so

davecheney commented 7 years ago

Blocked on #34.

I'd like to wait until issue #41 is resolved to make sure that changing the flag handling package does not affect #34.

n10v commented 7 years ago

It will not affect #34, but with pflag package it will be even easier to implement -H flags: with standard library you have to create your own value (how it did @freeformz), but in pflag you can do so:

var (
  ...
  httpHeaders []string
)

func init() {
  ...
  pflag.StringSliceVarP(&httpHeaders, "header, "H", "HTTP Header(s) to set. Can be used multiple times. -H 'Accept:...' -H 'Range:....'")
  ...
}
davecheney commented 7 years ago

Real talk: the three dependencies we have are unavoidable and do real work.

I'm not sold on the utility of the pflag package because it doesn't give me what I really want, a pretty usage() message. It just gives me another ugly one, and for that we have to pay for another dependency. I don't think that having GNU long args is enough of a benefit over what we have now.

I know I said previously that if you wanted to bring in a library to give long GNU args I would accept the patch and I'm sorry I'm pushing back on this now. It just doesn't feel to me like there's enough payoff for the cost of carrying that extra dependency.

n10v commented 7 years ago

It's not a problem with usage message. With pflag you can define your own usage if you want to. The reason because I imported this dependency is not only implementing long GNU flags, but with pflag you can faster and shorter implement flags, like the example with -H flag. And it's not so huge and slow as @moorereason said and it's true.

moorereason commented 7 years ago

I share your sentiments, Dave. If you don't want GNU args, then use the stdlib. I was shooting for curl flag compat, so I'm still in favor of using pflag.

freeformz commented 7 years ago

Being compatible with curl would be nice, but I personally don't have any other specific feelings around being compatible with GNU style long args. I am probably in the minority though. I am 👍 on not having another dep.

davecheney commented 7 years ago

I have decided to decline this PR. I don't think the benefit we get from have GNU long args is not offset by the cost of tracking another dependency.