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

Required Flags #85

Closed
slantview opened this issue Apr 29, 2014 · 44 comments · Fixed by #819
Closed

Required Flags #85

slantview opened this issue Apr 29, 2014 · 44 comments · Fixed by #819

Comments

@slantview
Copy link

It would be really nice to have required flags and required arguments. I'm mostly putting this here as a place holder to discuss although I would be willing to put some work in on this if we can talk about how we would like to implement it.

I was thinking the easiest would just add a new field to the struct called "Required". Internally we could have some checking and fail if the required flag is not set.

type Flag interface {
    fmt.Stringer
    Required bool
    // Apply Flag settings to the given flag set
    Apply(*flag.FlagSet)
    getName() string
}

func (f *Flag) IsRequired() bool {
  return f.Required
}
@johntdyer
Copy link

👍

@codegangsta
Copy link
Contributor

What is the use case for a required flag? I have never come across a cli app where a flag was required

@johntdyer
Copy link

Thanks for the quick response. Regarding the request, I cam think of dozens of reasons to define a flag as required. username, url, password, ect ...

@codegangsta
Copy link
Contributor

Just like required commands, this may be a better thing to handle in your application logic.

Also, adding a field to the Flag interface will unfortunately break the interface and all existing implementations.

On Jul 3, 2014, at 11:24 AM, John Dyer notifications@github.com wrote:

Thanks for the quick response. Regarding the request, I cam think of dozens of reasons to define a flag as required. username, url, password, ect ...


Reply to this email directly or view it on GitHub.

@jszwedko
Copy link
Contributor

I agree with @codegangsta here, there is no reason a flag should be "required". You should provide sensible defaults for the flag or instead read the value in as a command line argument.

@slantview
Copy link
Author

We wrote a tool called "motherbrain" (https://github.com/RiotGames/motherbrain) and we have use cases where an environment flag is required. Just because you don't have a case for it, doesn't mean they don't exist. What is the opposition to adding a way to require a flag? If you don't have that use case, simply don't use the feature. We really like this library, but there is no reason not to add features just because you don't need that particular feature.

Like I said in the original message, it would be really nice to have both required flags and required arguments. Every time I add a required global argument, I have to add a significant amount of boiler plate to each and every action function.

Edit: to clarify, we are willing to contribute code, but I was hoping this would be more of a discussion of how you wanted to implement, not "I don't need that feature, so I don't want it in here."

@slantview
Copy link
Author

The (sort-of) Ruby equivalent library for writing command line arguments has a required option for flags https://github.com/erikhuda/thor/wiki/Method-Options

@codegangsta
Copy link
Contributor

@slantview thanks for the example. I can see the use case for it now.

Unfortunately the proposed solution adds a method to the Flag interface. This will break other implementations of Flag and we can't do that unless we bump the package up to 2.0.

Maybe a utility function on context that will assert that a flag is set would be a better solution. Essentially it is just checking that a generic flag is not nil:

if c.Generic("flagname") == nil {
  log.Fatalln("flagname is required")
}

Although it is only 3 lines of code so it might not be as valuable to have in the codebase. Maybe useful to have in the readme?

@jszwedko
Copy link
Contributor

@slantview fair enough :)

@wjessop
Copy link

wjessop commented Aug 18, 2014

Most of the CLIs I write have required flags, would be great to have that handled in the package rather than re-implementing it each time.

@ahall
Copy link

ahall commented Aug 21, 2014

+1

@sugarb
Copy link

sugarb commented Aug 22, 2014

I think this applies to things like StringFlag more than Generic. For example, I have 3 files I need to specify for a command. The flags don't seem to get printed for subcommand help, and adding 9 lines to handle it is a bit much. Especially since you need to have a Value in order to get the help to display properly.

Edit: Nevermind about subcommand help, I was just using it wrong :) At the same time, required flags is a useful feature IMO.

@parnurzeal
Copy link

+1 totally agree. Required flags are very useful.

@enmand
Copy link

enmand commented Oct 28, 2014

+1

@tugberkugurlu
Copy link

👍 for this.

I agree with @codegangsta here, there is no reason a flag should be "required". You should provide sensible defaults for the flag or instead read the value in as a command line argument.

I'm developing a load testing cli tool and you need at least provide resource name which is --url flag.

Also, it should blow up the process if the required flag is not set.

@spagettikod
Copy link

+1

I was looking for this feature and ended up here. Just as @slantview I also have, what I feel, unnecessary boilerplate code to make sure flags / parameters are set. For example database connection strings, user names, passwords and so on.

@ivey ivey mentioned this issue Dec 2, 2014
@jhiemer
Copy link

jhiemer commented Mar 31, 2015

+1

2 similar comments
@assimovt
Copy link

assimovt commented Apr 7, 2015

👍

@bbiskup
Copy link

bbiskup commented Jun 10, 2015

+1

@stevvooe
Copy link

@codegangsta I am going to go against the tide here: if a flag is required, it should be a positional argument. Having required flags in the package promotes this anti-pattern. The suggestion in #85 (comment) covers the use case. Perhaps, some helper functions could be added to cover the required use case, as well as cases where certain flags are required when others are set.

@szank
Copy link

szank commented Jun 11, 2015

I did check comment #85 (comment), and for a StringFlag named "comment" I get :

fmt.Printf("comment : %#v\n", c.Generic("comment"))
#run app
./go run main.go
#result
comment : (*flag.stringValue)(0xc20802a9f0)

Is there any other way to check if flag is set ?
I wouldn't mind forking the code, adding 'required' parameter and using my own for from now onwards,
but we are using glide which depends on cli package and we get some strange glide errors if we use different cli build in our project.

@frodopwns
Copy link

I'd like to see a required flag that either automatically prints the command help when not present or prompts the user for the value.

@bwiggs
Copy link

bwiggs commented Jan 22, 2016

Also looking for this feature.

As for @stevvooe positional arguments: This becomes unfriendly to the user if there are more than a few required parameters. Imaging a cli app that needs to connect to a database, like psql. That would require a bunch of required parameters like username, password, host, port. If these were to be positional arguments, then the user would have to remember what order to append these when running the CLI. Using required params has the added benefit of providing labels for the user. This is more user friendly.

ex:

$ psql localhost postgres postgres 5434
vs
$ psql -h localhost -d postgres -U postgres -p 5434

@jszwedko jszwedko added the help wanted please help if you can! label Jan 22, 2016
@jszwedko
Copy link
Contributor

@bwiggs while I concede that this feature has some uses, your example of psql is slightly misleading because there are defaults for all of those flags, i.e. running psql with no args is the same as:

psql -h /var/run/postgresql -d jszwedko -U jszwedko -p 5432 (using my username as an example).

I think, wherever possible, flags with default values are a better approach than required flags, but I can imagine use cases (some of which have been described above) where there are no sensible defaults and the value must be provided by the user.

I'm happy to review any pull request introducing this behavior (see #155 for a possible start).

Thanks!

@owenthereal
Copy link

+1 to required flags

@stevvooe
Copy link

As for @stevvooe positional arguments: This becomes unfriendly to the user if there are more than a few required parameters. Imaging a cli app that needs to connect to a database, like psql. That would require a bunch of required parameters like username, password, host, port. If these were to be positional arguments, then the user would have to remember what order to append these when running the CLI. Using required params has the added benefit of providing labels for the user. This is more user friendly.

@bwiggs pointed it out, but I wanted to expand on what I said above. psql is a perfect example of great cli design. With no arguments, it does something. username, password, host and port are not required parameters in psql. The only positional parameter is the database and username, which are not required. I can configured my environment or .pgsql such that the correct configuration can be picked up.

There are use cases, such as mutually exclusive parameters. Most of the time, these are just easier to implement directly in the command itself, as there may be a lot of application logic to make that decision and emit the correct error.

My main issue with required options is that we too often see tools like this:

cmd subcmd --path foo

Why should I have to write --path when it could be provided by a positional parameter? One may ask, what about more required parameters? Consider a way to do it with less parameters. I've never enjoyed using a tool with more than 3 required parameters that I enjoyed using. Notice that 3 is well within the bounds of using positional arguments. In tools that require such a ridiculous number of parameters (and they do sometimes), more than half of them tend to be the same on each invocation. At that point, just make a configuration file or pull options from the environment.

Required options just tend to lead to bad CLI design.

@charneykaye
Copy link

charneykaye commented Apr 22, 2016

+1.

I understand that for many, CLI primarily means a tool used by a single developer on the command line.

However, please consider applications that are deployed using other means yet still take "command line" arguments. My use case is an application that hosts a micro-service with HTTP endpoints and connections to databases. I have three required flags:

OPTIONS:
   --port                               REQUIRED <number> port to serve HTTP endpoints
   --redis                              REQUIRED <host:port> of Redis DB
   --mysql                              REQUIRED <user:pass@host:port/dbname> of MySQL DB

Although I appreciate the flip perspective, that perhaps by default such an application ought to use the developer team local environment defaults:

OPTIONS:
   --port                               <number> port to serve HTTP endpoints, default: 80
   --redis                              <host:port> of Redis DB. default: localhost:6379
   --mysql                              <user:pass@host/dbname> of MySQL DB, default: root@localhost:3306/app

@obourdon
Copy link

+1

Most CLI libraries for other languages do support required arguments to ease up burden of CLI development and focusing on real added value ;-)

https://commons.apache.org/proper/commons-cli/apidocs/index.html
http://jcommander.org/#Required_and_optional
https://pholser.github.io/jopt-simple/examples.html

@wottah
Copy link

wottah commented Jul 1, 2016

+1

1 similar comment
@asoseil
Copy link

asoseil commented Jul 24, 2016

+1

@krancour
Copy link

My biggest argument in favor of this feature is that sometimes a command or subcommand may require many inputs. Allowing these inputs to be provided as the values of flags (as opposed to arguments) has a benefit in that no one needs to be aware of or honor a specific order in which they are entered. If one were to use flags in this manner, it follows that one would want the ability to define a flag as required.

+1

@jszwedko
Copy link
Contributor

jszwedko commented Sep 3, 2016

Agreed, #155 attempts to address this, but I haven't had the time to circle back around to finish the remaining issues with it.

@deepujain
Copy link

+1

@sirkon
Copy link

sirkon commented Oct 5, 2017

Github needs a way to put an unstar to a project. Lack of --required and further mindless opposition to this option deserves to be downvoted.

@gtaylor
Copy link

gtaylor commented Oct 5, 2017

@sirkon This software is provided for free for yours (and everyone else's) benefit. There is nothing stopping you from contributing to the situation constructively by submitting a PR.

@brunson
Copy link

brunson commented Oct 23, 2017

#155 - Required flags

This MR is about to turn 3 years old. We should organize a party.

@jszwedko
Copy link
Contributor

jszwedko commented Oct 24, 2017

❤️ 🎊 🎺

I'd love to see this feature added as there appears to be a pretty clear need. I unfortunately don't have the time lately to contribute it myself, but I'm happy to review PRs (I recommend starting from #155).

@wencan
Copy link

wencan commented Jul 6, 2019

+1

@coilysiren
Copy link
Member

Oh hi 👋 I just noticed the helpwanted tag! I use this packages quite extensively @ work, and just finished a PR (sirupsen/logrus#977) to a package I use in a similar context. So I'd like to help with this!

I'll start from here #155 👍 Via creating a branch off on the most recent commit, rebasing, and working through the review comments 🔧

This was referenced Jul 8, 2019
@coilysiren
Copy link
Member

I've started on this over here ~> #819 feel free to drop me a comment if there's anything large that I've missed!

@coilysiren
Copy link
Member

If anyone is interested in helping with this, come check out my PR here #819. It's ready for review 👀

@coilysiren
Copy link
Member

FYI for anyone that's only following this GitHub issue - I added this feature to the latest release https://github.com/urfave/cli/releases/tag/v1.21.0

@coilysiren coilysiren removed the help wanted please help if you can! label Aug 10, 2019
@marwan-at-work
Copy link
Contributor

@lynncyrin can we re-open this issue for the v2 branch? :)

@coilysiren
Copy link
Member

coilysiren commented Aug 24, 2019

@marwan-at-work we'll be bringing v2 up to date "automatically" via merging master into it. Open issues are generally reserved for unique instances of work, that won't happen via other means.

So for instance, there would be a "merge master into v2" issue - but not a "merge feature 1, feature 2, feature 3, ... into v2" set of issues. The idea is that the "merge feature" issues essentially duplicate the "merge master" issue.

So I just updated the V2 release tracker issue (#826) to reflect the fact that nobody is currently working on bringing V2 up to date with master, and that we could use some help with that. If nobody picks it up, I'll work on it after I fix #850.

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 a pull request may close this issue.