akamensky / argparse

Argparse for golang. Just because `flag` sucks
MIT License
604 stars 62 forks source link

enable required for positionals #108

Open vsachs opened 2 years ago

vsachs commented 2 years ago

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

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

ryansch80 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 https://github.com/akamensky/argparse/issues/108#issuecomment-1259879996 it should be considered a bug rather than an enhancement.

vsachs commented 2 years ago

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 commented 1 year ago

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

vsachs commented 1 week ago

@Outragedpillow yes, although as you can see we're both easily distracted, I continuously think to myself I need to get back to this stuff

I will try to do so this month or in october, sorry for the... hilariously long delays