Skip to content

improve compatibility with go/flag#241

Closed
CosmicToast wants to merge 1 commit into
spf13:masterfrom
CosmicToast:compat
Closed

improve compatibility with go/flag#241
CosmicToast wants to merge 1 commit into
spf13:masterfrom
CosmicToast:compat

Conversation

@CosmicToast
Copy link
Copy Markdown

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

@mckern
Copy link
Copy Markdown

mckern commented Feb 17, 2020

👋 howdy! Some of this looks like it overlaps with my existing PR #220; do you want to pull my commits into your PR?

go/flag in go 1.13 has a public Output() function

Following up on this, it looks like Output has been exported for about two years now so that'd put it around Go 1.10; definitely pretty safe for reasonable backwards compatibility (I think).

@CosmicToast
Copy link
Copy Markdown
Author

howdy! Some of this looks like it overlaps with my existing PR #220; do you want to pull my commits into your PR?

Absolutely!
I'm pretty extremely busy right now (unfortunate circumstances) - I'll try and get them in Wed evening, or Thu morning at the latest.
Seeing as this has been sitting here for almost 3 weeks now, I doubt that'll have much of an effect.

Following up on this, it looks like Output has been exported for about two years now so that'd put it around Go 1.10

I'm also considering simply forking and sprucing everything up at this point, for basically the same reason.
I suspect pflag doesn't get the attention it deserves because Steve also maintains cobra and co, so it'd be nice to give it a better home 🙂
If you're interested we can try and figure something out to that effect.

@cornfeedhobo
Copy link
Copy Markdown

Re: giving pflag a better home:

the problem is that cobra doesn't expose a way to drop in another flag library ... unless you all know a trick I don't?

@CosmicToast
Copy link
Copy Markdown
Author

the problem is that cobra doesn't expose a way to drop in another flag library ... unless you all know a trick I don't?

I am wholly disinterested in cobra.
I am interested in pflag as it is, or at least was originally, in its time.
Wrapping around it is also of interest to me, but in a way that significantly differs from cobra.

Forks need not only exist as ways to change some "definitive upstream" pointer.
Forks can exist for the same (or quite similar) software to serve differing purposes.
If this pflag is intended to be a backend provider for cobra, so be it - I would then maintain the pflag meant for direct use.

@eparis
Copy link
Copy Markdown
Collaborator

eparis commented Feb 19, 2020

I don't think cobra or viper or any of the other packages steve owns matter here. We're just busy and would love anyone who is consistently able to show good judgement and help maintain things.

In this case I'm totally fine with the changes. If @5paceToast and @mckern agree on a PR that is correct I'll merge it.

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).
@CosmicToast
Copy link
Copy Markdown
Author

@mckern do you think the merged changes do you sufficient justice?

@mckern
Copy link
Copy Markdown

mckern commented Feb 20, 2020

@5paceToast not quite; there's a test in my commit that's missing here. I took a look at your commit, and cherry-picked it onto mckern/output, which preserved the test for Out(), as well as your tests for Name(). I also took a turn at moving your test into its own function instead of adding it to an existing one (and ensured you were credited as co-author on the commit for it). How does that look to you?

@eparis thanks for checking in -- I know y'all are volunteers, and I appreciate your time, attention, and efforts.

@CosmicToast
Copy link
Copy Markdown
Author

Closing in favor of #220.

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.

4 participants