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

proposal: flag: support Retired flags #32015

Closed
dgryski opened this issue May 13, 2019 · 13 comments
Closed

proposal: flag: support Retired flags #32015

dgryski opened this issue May 13, 2019 · 13 comments

Comments

@dgryski
Copy link
Contributor

dgryski commented May 13, 2019

The abseil library recently added support for retired flags: https://abseil.io/docs/cpp/guides/flags#removing--retiring-flags

Similar functionality should be added to the Go flag package.

@ianlancetaylor ianlancetaylor changed the title flag: support Retired flags proposal: flag: support Retired flags May 13, 2019
@gopherbot gopherbot added this to the Proposal milestone May 13, 2019
@neild
Copy link
Contributor

neild commented May 14, 2019

FWIW, we've got an internal retiredflag package with:

func NewBool(name string) {}
func NewNonBool(name string) {}

The bool/non-bool distinction is necessary since -x y is parsed differently depending on whether -x is boolean or not. The NewNonBool function was originally named retiredflag.New, but this proved to be confusing; many ex-boolean flags were incorrectly registered as non-bool.

I see that the ABSL version maintains the full type information of the flag and continues to check values for syntactic correctness; that might be a superior way to go.

@peterbourgon
Copy link

The linked guide says that retired flags:

  1. do not define a C++ FLAGS_ variable.
  2. have a type and a value, but that value is intentionally inaccessible.
  3. do not appear in --help messages.
  4. are fully supported by all flag parsing routines.
  5. consume args normally, and complain about type mismatches in those arguments.
  6. emit a complaint but do not die if they are accessed by name through the flags API for parsing or otherwise.

Of those properties, only 2, 3, and 5 would apply to Go. But all of those applicable properties, except 3, are already possible today if applications simply define but do not use a flag. Consequently, I don't think this feature would carry its own weight.

@robpike
Copy link
Contributor

robpike commented May 14, 2019

@peterbourgon is right. The benefits are mild at best and don't seem worth the complexity and API surface.

@beoran
Copy link

beoran commented May 15, 2019

The solution to this problem, would be to define a DeprecatedValuetype that implements the Value interface and wraps the method calls of an existing Value but emits warnings when they are called. Although this is very simple, I do not know whether or not such a DeprecatedValue needs to be in the standard library or not.

@rsc
Copy link
Contributor

rsc commented May 28, 2019

As Peter said, a user-defined flag.Value implementation can do all of this already, except not showing up in help text. I wonder if we should define that deprecated flags have help text beginning with "deprecated:" (or "Deprecated:"?), and then flag.PrintDefaults can maybe omit those. But if an old implementation is used, or we don't do any omitting, it will be clear enough to users that the flag is no longer relevant.

@jimmyfrasche
Copy link
Member

@rsc If I deprecate a flag I usually want to still show it for a version or so but warn that it is deprecated and explain the alternatives to give users/documentation time to adjust.

There are also cases where there are magic debugging flags or flags that should only be used if the binary is execing itself, etc.. None of those are useful to list in PrintDefaults.

A method on flag set that takes a ...string of flag names to exclude from PrintDefaults would cover both those cases.

@rsc
Copy link
Contributor

rsc commented May 28, 2019

There are also cases where there are magic debugging flags or flags that should only be used if the binary is execing itself, etc.. None of those are useful to list in PrintDefaults.

I don't know; I am not as excited about hiding "flags that exist and have magical meanings but that I'd prefer not to talk about."

It's possible we should do nothing at all here in the flag package but still establish the "deprecated:" prefix, in the spirit of #10909.

@rsc
Copy link
Contributor

rsc commented Jun 4, 2019

@ianlancetaylor points out that any special behavior in PrintDefaults triggered by a prefix like "deprecated" interacts badly with flag descriptions written in non-English languages.

We should probably establish the deprecated convention and stop there, without making any changes to the flag package. Does that seem OK?

@jimmyfrasche
Copy link
Member

Who benefits from that convention? Is it supposed to be the for the users of the program written using the flag package?

@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

Yes, mainly users of the program benefit. It's just like putting "Deprecated:" on a package or a function (see #10909). It's a signal for people.

@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

Based on discussion above I propose to close this proposal, but @dgryski has been quiet and I'd like to hear more from him about what he thinks now.

@dgryski
Copy link
Contributor Author

dgryski commented Jun 11, 2019

I'm fine to close. I just saw the blog post from abseil :) . I have no immediate need for this.

@rsc
Copy link
Contributor

rsc commented Jun 25, 2019

Thanks @dgryski. Closing.

@rsc rsc closed this as completed Jun 25, 2019
@golang golang locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants