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: add url.URL flag type #52622

Closed
YaroslavDev opened this issue Apr 29, 2022 · 12 comments
Closed

proposal: flag: add url.URL flag type #52622

YaroslavDev opened this issue Apr 29, 2022 · 12 comments

Comments

@YaroslavDev
Copy link

Add url.URL type of flag to existing standard flags implemented in flag package.
Essentially, I want to implemente 4 funcs in flag/flag.go:

func URLVar(p *url.URL, name string, value url.URL, usage string) {
func (f *FlagSet) URL(name string, value url.URL, usage string) *url.URL {
func URL(name string, value url.URL, usage string) *url.URL {
func (f *FlagSet) Var(value Value, name string, usage string) {

This would allow using it in calling code like so:

fs.URLVar(&authURL, "auth-url", url.URL{}, "URL to auth service")
@gopherbot gopherbot added this to the Proposal milestone Apr 29, 2022
@seankhliao
Copy link
Member

how common is this that it needs to be in the stdlib?

@ianlancetaylor ianlancetaylor changed the title proposal: flag: Add url.URL flag type proposal: flag: add url.URL flag type Apr 29, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 29, 2022
@YaroslavDev
Copy link
Author

@seankhliao it can be quite handy for passing URLs as flags to binaries. String argument will be parsed using url.Parse and converted to url.URL type. This flag will be very similar to existing time.Duration flag.

@ianlancetaylor
Copy link
Contributor

Thanks, but handy is not the same as common.

A flag like this doesn't need to live in the standard library. It can be implemented entirely outside of the standard library by implementing the flag.Getter interface. The only reason that we have flag.Duration in the flag package is that we don't want to put high level concepts like flags in the very low level time package.

So the question here is: what makes this flag important enough to put into the standard library?

Thanks.

@YaroslavDev
Copy link
Author

@ianlancetaylor
AFAIK, you have to create your own type in order to implement flag.Value and if you have other code that expects url.URL(not your custom type), you have to explicitly convert it like so url.URL(myFlagVar). AFAIK, you can't implement interfaces on types that you do not define. In this case url.URL type just as time.Duration are types of stdlib, and I, as a developer of web services, find it very common to provide URLs as configs to my programs.

DISCLAIMER: It's just my opinion, and I may not see a bigger picture here, or maybe there is a specific reason for not including other stdlib types to flag package.

@uluyol
Copy link
Contributor

uluyol commented Apr 29, 2022

Wouldn't it be enough to implement TextMarshaler/Unmarshaler for url.URL?

@YaroslavDev
Copy link
Author

@uluyol your suggestion seems reasonable to me. I didn't know smth. like this was implemented. It's not released yet, right?

@uluyol
Copy link
Contributor

uluyol commented Apr 30, 2022

It looks like flag.TextVar will be part of Go 1.19.

@YaroslavDev
Copy link
Author

@uluyol cool, but it's not a blocker to implement TextMarshaler/Unmarshaler interfaces for url.URL right now?

@uluyol
Copy link
Contributor

uluyol commented Apr 30, 2022

1.19 is the next release, so any change to url will, at the earliest, be in the same release.

@YaroslavDev
Copy link
Author

OK, do I need more approvals for implementing TextMarshaler/Unmarshaler interfaces for url.URL?
shall we discuss it in this proposal or should I create a new one?

@ianlancetaylor
Copy link
Contributor

Please open a new issue (and close this one if you don't want it any more). Thanks.

@YaroslavDev
Copy link
Author

closed in favor of #52638

@ianlancetaylor ianlancetaylor removed this from Incoming in Proposals (old) May 2, 2022
@golang golang locked and limited conversation to collaborators May 1, 2023
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

5 participants