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 exported functions to preserve pkg/flag compatibility #220

Merged
merged 3 commits into from
May 4, 2020
Merged

Add exported functions to preserve pkg/flag compatibility #220

merged 3 commits into from
May 4, 2020

Conversation

mckern
Copy link

@mckern mckern commented Sep 18, 2019

These changes bring behavior inline with go's flag library, and allows for printing output directly to whatever the current FlagSet is using for output and retrieving a given FlagSet's name. These changes will make it easier to correctly emit output to stdout or stderr (e.g. a user has requested a help screen, which should emit to stdout since it's the desired outcome).

(This PR has also pulled in #241, since @5paceToast and I were clearly working in the same direction here)

example for Out():

import (
  "fmt"
  flag "github.com/spf13/pflag"
)

func init() {
  var helpFlag bool
  whoami := path.Base(os.Args[0])
  flag.BoolVarP(&helpFlag, "help", "h", false, "show this help")

  flag.Usage = func() {
    fmt.Fprintf(flag.CommandLine.Output(), "%s: print example help\n\n", whoami)
    fmt.Fprintf(flag.CommandLine.Output(), "usage: %s [-h]\n", whoami)
    flag.PrintDefaults()
  }

  flag.Parse()

  if helpFlag {
    flag.CommandLine.SetOutput(os.Stdout)
    flag.Usage()
    os.Exit(0)
  }
}

example for Name():

import (
  "fmt"
  flag "github.com/spf13/pflag"
)

func init() {
  versionNumber := "123.456.789"
  myFlagSet := flag.NewFlagSet("Megumin", flag.ExitOnError)

  fmt.Printf("%s\t%s\n", myFlagSet.Name(), versionNumber)
}

This brings behavior inline with go's flag library, and allows for
printing output directly to whatever the current FlagSet is using for
output. This change will make it easier to correctly emit output to
stdout or stderr (e.g. a user has requested a help screen, which
should emit to stdout since it's the desired outcome).
@CLAassistant
Copy link

CLAassistant commented Sep 18, 2019

CLA assistant check
All committers have signed the CLA.

CosmicToast pushed a commit to CosmicToast/pflag that referenced this pull request Feb 19, 2020
go/flag in go 1.13 has a public Output() function and a Name() function
- change out() to Output() (adjusting the rest of the file)
- add Name() and tests for it

From: @mckern (spf13#220) re: out/Output:
This brings behavior inline with go's flag library, and allows for
printing output directly to whatever the current FlagSet is using for
output. This change will make it easier to correctly emit output to
stdout or stderr (e.g. a user has requested a help screen, which
should emit to stdout since it's the desired outcome).
pkg/flag has a public `Name()` function, which returns the name of the
flag set when called. This commit adds that function, as well as a
test for it.
@mckern mckern changed the title Rename out() to Output() (to export it) Add exported functions to preserve pkg/flag compatibility Feb 20, 2020
@CosmicToast
Copy link

I'd prefer if the testName was capitalized (as is the standard, I'm not sure it would even run in this state) -> TestName.
Besides that LGTM!

@mckern
Copy link
Author

mckern commented Feb 20, 2020

Oh it definitely runs (I induced a failure state to check). It doesn't look like the standard is consistently applied and I'm not sure why some tests end up as public functions and some don't:

ryan@bender pflag (git:output) $ ag -c -s -Q 'func Test' flag_test.go
35
ryan@bender pflag (git:output) $ ag -c -s -Q 'func test' flag_test.go
5

Looks like more of them are public than not though, so yeah, let's go with that.

Testing `Name()` will move into its own explicit test, instead of
running inline during `TestAddFlagSet()`.

Co-authored-by: Chloe Kudryavtsev <[email protected]>
@mckern
Copy link
Author

mckern commented Feb 20, 2020

/cc @eparis I think we've worked it out, if you're still available for review? Please & thank you!

@mckern
Copy link
Author

mckern commented Mar 6, 2020

Ping @eparis ?

@mckern
Copy link
Author

mckern commented Apr 24, 2020

Ping @eparis and/or @therealmitchconnors ?

@wojtekk-ac
Copy link

just 3cents, I am kind of waiting for that to be in pflag since literally years. (had sibling issue #230 opened here)

@therealmitchconnors therealmitchconnors merged commit 81378bb into spf13:master May 4, 2020
@wojtekk-ac
Copy link

Thank you @therealmitchconnors !

@mckern
Copy link
Author

mckern commented May 4, 2020

thanks @therealmitchconnors 🎉 !

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.

5 participants