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

argparse says argument is optional when it's required #8055

Open
rlipscombe opened this issue Jan 26, 2024 · 7 comments
Open

argparse says argument is optional when it's required #8055

rlipscombe opened this issue Jan 26, 2024 · 7 comments
Assignees
Labels
bug Issue is reported as a bug help wanted Issue not worked on by OTP; help wanted from the community team:VM Assigned to OTP team VM

Comments

@rlipscombe
Copy link
Contributor

rlipscombe commented Jan 26, 2024

Describe the bug
Usage text for 'required' arguments has them listed under "Optional arguments".

To Reproduce

cli() ->
    #{
        arguments => [
            #{
                name => server,
                long => "-server",
                % Note 'required'
                required => true,
                nargs => nonempty_list,
                action => extend,
                type => {custom, fun parse_server/1}
            }
        ],
        % ...

Usage reads thus:

...
Optional arguments:
  --server server

Expected behavior

Usage should probably read thus:

...
Required arguments:
  --server server

Affected versions

OTP-26.2.1

@rlipscombe rlipscombe added the bug Issue is reported as a bug label Jan 26, 2024
@rlipscombe
Copy link
Contributor Author

Note that this isn't a minimal repro; I'll work on paring it down on Monday.

@jhogberg jhogberg added the help wanted Issue not worked on by OTP; help wanted from the community label Jan 26, 2024
@jhogberg jhogberg self-assigned this Jan 26, 2024
@jhogberg
Copy link
Contributor

Thanks, we'll look into it when we find the time, though it may take a while with 27-rc1 and 26.3 coming up soon, so we'd welcome a PR if anyone's inclined :)

@rlipscombe
Copy link
Contributor Author

I might get to it next week, but no promises.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jan 29, 2024
@MarkoMin
Copy link
Contributor

MarkoMin commented Feb 9, 2024

I've spent fair amount of time today on argparse module and after reading the source I'm not sure if this is even a bug. argparse have 2 types of arguments: positional and optional. If argument has short or long defined, it is considered optional. However, line 85 in the source says that if required is true then it "markes parser to return an error if the argument is not present in the command line", but this doesn't change the fact that server is still an optional. You can see it in format_opt_help/2, where any optional argument is appended to OptL accumulator no matter if required is set or not. What we have here is an example of "required optional argument".

When printing help message, would it make sense to split arguments based on the "required" value, rather than by "is it positional or optional"? Also renaming "optional arguments" to "option" or "option argument" could make confusion smaller.

What I also found strange is that in argparse_SUITE, line 117 there is optional argument that hasn't required attribute set, but it's help message says "required optional, right?".

I think it's suitable to ping the author here @max-au

EDIT:
Same situation happened before: #7964

@max-au
Copy link
Contributor

max-au commented Feb 16, 2024

Now I see how this feature (allowing options to be required = true) can result in somewhat confusing "usage" output. It was supposed to be rarely needed (I even contemplated keeping it undocumented), but looks like it's got far more usage than I originally anticipated.

Also renaming "optional arguments" to "option" or "option argument" could make confusion smaller.

I'm fine with this change, although, as soon as the code has been contributed, the decision must be approved by the OTP team.

What I also found strange is that in argparse_SUITE, line 117 there is optional argument that hasn't required attribute set, but it's help message says "required optional, right?".

Oh, that's indeed a (copy-pasted) typo :-D the actual test is below, for --yyy parameter. That change is non-contradictory, I'll just include that in a PR.

@max-au
Copy link
Contributor

max-au commented Feb 16, 2024

When printing help message, would it make sense to split arguments based on the "required" value, rather than by "is it positional or optional"?

As for that, I don't have an opinion. Original implementation was based on the premise of sorting arguments into positional and optional (prefixed). Migrating existing apps to argparse clearly discovered plenty of apps not following this pattern. They had optional arguments (started with prefix) as required. I think it's weird on its own, and quite confusing, - this argument should not be an option in the first place.

@rlipscombe
Copy link
Contributor Author

They had optional arguments (started with prefix) as required. I think it's weird on its own, and quite confusing, - this argument should not be an option in the first place.

The problem with positional arguments is that you need to remember which order they come in. Whereas if you consider the --whatever as a label for the argument, the other viewpoint makes sense.

It's not positional vs. optional. It's positional vs. named. Positional arguments can also be optional (i.e. if you just omit the Nth, N-1th, etc. arguments, use defaults).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug help wanted Issue not worked on by OTP; help wanted from the community team:VM Assigned to OTP team VM
Projects
None yet
Development

No branches or pull requests

5 participants