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.

Sign up for GitHub

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

Jump to bottom

flag: add BoolFunc(name, usage string, fn func(string)error) #53747

Closed
fnxpt opened this issue Jul 8, 2022 · 25 comments
Closed

flag: add BoolFunc(name, usage string, fn func(string)error) #53747

fnxpt opened this issue Jul 8, 2022 · 25 comments
Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Go1.21

Comments

@fnxpt
Copy link

fnxpt commented Jul 8, 2022

Using flag.Func forces you to always pass an argument. Sometimes we don't really need an argument and its really convenient to be able to just call this flags without passing any dummy value.

Example:
Setting the flag --start-server will ask you to specify a value and also mentions it on the usage
-start-server value
Starts the server
@fnxpt fnxpt added the Proposal label Jul 8, 2022
@gopherbot gopherbot added this to the Proposal milestone Jul 8, 2022
@mvdan
Copy link
Member

mvdan commented Jul 8, 2022

You mean that you want https://pkg.go.dev/flag#Bool?

@fnxpt
Copy link
Author

fnxpt commented Jul 8, 2022

No, Im aware that bool exists, but if you need to have a function in a flag you need to always set a value... Im preparing a PR for this

fnxpt pushed a commit to fnxpt/go that referenced this issue Jul 8, 2022
flag: support func flags without arguments
8148747 
    Current implementation forces a func flag to have a value
    When displaying the usage is possible to see a "value" required for the flag and when you execute it without flag you receive the error flag needs an argument

    Converted funcValue to a struct and create 2 additional functions to avoid breaking changes

    Fixes golang#53747
fnxpt pushed a commit to fnxpt/go that referenced this issue Jul 8, 2022
flag: support func flags without arguments
635f52e 
    Current implementation forces a func flag to have a value
    When displaying the usage is possible to see a "value" required for the flag and when you execute it without flag you receive the error flag needs an argument

    Converted funcValue to a struct and create 2 additional functions to avoid breaking changes

    Fixes golang#53747
@fnxpt fnxpt mentioned this issue Jul 8, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/416514 mentions this issue: flag: support func flags without arguments

@fbiehl
Copy link

fbiehl commented Jul 8, 2022

This is a really welcome change!!
Having immediate custom handling function as the flag is detected is very useful.
Currently this is just not possible for flags that do not require a value.

@seankhliao seankhliao changed the title proposal: flag: allow Func flags without arguments proposal: flag: add FuncNoArg for Func flags without arguments Jul 8, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 8, 2022
@earthboundkid
Copy link
Contributor

I added flag.Func. I also wrote my own helper function that works like your proposal: https://pkg.go.dev/github.com/carlmjohnson/flagx#BoolFunc I’m not totally convinced it belongs in the standard library though. So far it has been less useful than I thought it would be. It’s only slightly more convenient than just using a flag.Bool.

@fnxpt
Copy link
Author

fnxpt commented Jul 9, 2022 via email •
edited

@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

In the CL, it's not that there's no argument, it's that you must use =value.
In the flag package that's called a boolflag, like IsBoolFlag.
So if this goes in, it should be BoolFunc, right?

@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 13, 2022
@earthboundkid
Copy link
Contributor

Should it be BoolFunc(name, usage string, fn func() error) or BoolFunc(name, usage string, fn func())?

@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

It should be func(string) error like the others, because you can say

cmd -name -name=true -name=false -name=auto

The same as a custom flag implementation that returns true from IsBoolFlag.

@rsc rsc changed the title proposal: flag: add FuncNoArg for Func flags without arguments proposal: flag: add BoolFunc(name, usage string, fn func(string)error) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

Updated title. Does anyone object to adding this?

@fnxpt
Copy link
Author

fnxpt commented Jul 20, 2022

Should I update the code to reflect these changes before the proposal review?

@seankhliao
Copy link
Member

While this can already be done via implementations that have IsBoolFlag, I'm not sure I'd appreciate this becoming more convenient/widespread given that you can pass an arbitrary value to it.
It leads to unintuitive/unpredictable CLIs with some flags that require an = to pass an argument.

@earthboundkid
Copy link
Contributor

I'm +0 on adding it. As I said, I have a version of this in my personal toolbox and so far I have been underwhelmed with how useful it is. On the other hand, it rounds out the functionality in a pretty clear way and it doesn't harm anything to have it. I do think though that if the callback signature ends up being func(string) error, I probably will keep using my version of the helper because I don't want to have to do the rigamarole to convert string to bool every time I use it.

@ianlancetaylor
Copy link
Contributor

@fnxpt Accepting or rejecting the proposal is independent of the sample implementation. So, sure, update it, but no rush.

@rsc
Copy link
Contributor

rsc commented Jul 27, 2022

It seems like this is missing and fills in a hole that people can fall into otherwise. So even though it's not going to be an everyday kind of function, it seems worth adding.

@rsc
Copy link
Contributor

rsc commented Jul 27, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jul 27, 2022
@fnxpt
Copy link
Author

fnxpt commented Jul 30, 2022 •
edited

renamed the function to BoolFunc. Runned the tests and it fails on API check, in which file should I signature?

@earthboundkid
Copy link
Contributor

Read https://github.com/golang/go/blob/master/api/README

@fnxpt
Copy link
Author

fnxpt commented Aug 1, 2022

Thanks @carlmjohnson

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Aug 3, 2022
@rsc
Copy link
Contributor

rsc commented Aug 3, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: flag: add BoolFunc(name, usage string, fn func(string)error) flag: add BoolFunc(name, usage string, fn func(string)error) Aug 3, 2022
@rsc rsc modified the milestones: Proposal, Backlog Aug 3, 2022
@fnxpt
Copy link
Author

fnxpt commented Aug 11, 2022

@rsc what are the next steps. There is already a PR with the changes mentioned

@earthboundkid
Copy link
Contributor

@fnxpt Do you want to update the PR to fix the merge conflict, or are you not working on this anymore?

earthboundkid added a commit to earthboundkid/go that referenced this issue Mar 13, 2023
@earthboundkid
flag: add BoolFunc; FlagSet.BoolFunc
54bfa1d 
Fixes golang#53747
Supercedes #416514
@earthboundkid earthboundkid mentioned this issue Mar 13, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/476015 mentions this issue: flag: add BoolFunc; FlagSet.BoolFunc

earthboundkid added a commit to earthboundkid/go that referenced this issue Mar 14, 2023
@earthboundkid
flag: add BoolFunc; FlagSet.BoolFunc
daea1e0 
Fixes golang#53747

Based on [CL 416514](https://go.dev/cl/416514)
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Mar 16, 2023
@gopherbot
Copy link

Change https://go.dev/cl/499417 mentions this issue: doc/go1.21: mention flag.BoolFunc

gopherbot pushed a commit that referenced this issue May 31, 2023
@ianlancetaylor @gopherbot
doc/go1.21: mention flag.BoolFunc
d329fc5 
For #53747

Change-Id: Ia5e2f89c1184f2dfd6d672b838b0dbb579e6c954
Reviewed-on: https://go-review.googlesource.com/c/go/+/499417
Reviewed-by: Eli Bendersky <eliben@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@mknyszek mknyszek mentioned this issue Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
No open projects
Proposals (old)
Accepted
Milestone
Go1.21
Development

Successfully merging a pull request may close this issue.

flag: add BoolFunc; FlagSet.BoolFunc earthboundkid/go
9 participants
@rsc @earthboundkid @fnxpt @dmitshur @fbiehl @ianlancetaylor @mvdan @gopherbot @seankhliao