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

enable required for positionals #108

Open
vsachs opened this issue Sep 2, 2022 · 5 comments
Open

enable required for positionals #108

vsachs opened this issue Sep 2, 2022 · 5 comments

Comments

@vsachs
Copy link
Collaborator

vsachs commented Sep 2, 2022

Positionals (added in 1.4) currently ignore the Required option. It should be supported, with these basic requirements:

  • Required positionals must have a value supplied by the user (not by Default) or an error is thrown
  • If a positional is set to required, throw an error if there are already any positionals on this command which are NOT required
    -- This is necessary in order to avoid ambiguity of positional argument satisfaction: Say you have two positionals, the first is optional and the second is required. The user gives one value on the CLI. Which positional gets the value? Does the required positional get it IFF the optional has a default?
    -- Do not throw an error if there are optional positionals on a parent command of the command which has had Required positionals added
  • Allow optional positionals to follow Required positionals
  • Do not throw an error if there are Required positionals on commands which did not Happen

Some discussion of the problem is warranted before coding begins. This may not be an exhaustive list of requirements.

@ryansch80
Copy link

ryansch80 commented Sep 27, 2022

I'm not sure if this belongs here, or could be considered a separate issue:

I'm using argparse with 3 (required) positional args and 1 optional arg. If you don't pass any args to the program, error is nil and program execution continues (and eventually causes errors when trying to access non existent files):

parser := argparse.NewParser("", "blahblahblah")
var inFile *os.File = parser.FilePositional(os.O_RDONLY, 0600, &argparse.Options{Required: true, Help: "..."})
var outFilename *string = parser.StringPositional(&argparse.Options{Required: true, Help: "..."})
var configFile *os.File = parser.FilePositional(os.O_RDONLY, 0600, &argparse.Options{Required: true, Help: "..."})
startAtFileRow := parser.Int("r", "rowstart", &argparse.Options{Default: 1, Required: false, Help: "..."})

err := parser.Parse(os.Args)
if err != nil {
     log.Fatal(parser.Usage(err))
}

When I push this to a server for use by other users, most of them will naively execute it w/out any args nor even passing "-h" with the expectation that it will show them which arguments are required. In this scenario, the program just explodes:
$ ./asset-migrator
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4b63b1]
...

@akamensky
Copy link
Owner

akamensky commented Sep 29, 2022

@ryansch80 That sounds as a bug that needs to be assessed and fixed.

Yes, that seems to be related, if I understand top post correctly required currently ignored.

As a workaround until this is fixed you could add a check of positional values and throw an error if they are not provided.

@akamensky
Copy link
Owner

akamensky commented Sep 29, 2022

@vsachs Thanks, I think we do need to enable Required to work for positionals. I generally agree with all requirements except for:

Do not throw an error if there are Required positionals on commands which did not Happen

I think if positional is marked as required and there is no value available to fill it -- should throw an error.

I would probably say that as per #108 (comment) it should be considered a bug rather than an enhancement.

@akamensky akamensky added bug and removed enhancement labels Sep 29, 2022
@vsachs
Copy link
Collaborator Author

vsachs commented Sep 30, 2022

I think if positional is marked as required and there is no value available to fill it -- should throw an error.

If we always throw an error when a required is unsatisfied, then if someone creates multiple sub-commands all of which have a required positional, the argparser will always error. There will be no way for one sub-command to "take precedence". I think that's not a good system but it's up for discussion of course.

it should be considered a bug rather than an enhancement.
Sure, that's fine I guess.

I'll try to take a couple moments next week to fix this. I'm working on Nargs as well but I got pulled away from it.

@Outragedpillow
Copy link

I see these issues and comments are older. Are you still maintaining this repo and accepting PRs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants