-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Double-dash separator can be used between group name and commands, has odd behavior #619
Comments
Feel free to submit a PR with your proposed change. However, I think it would be hard to make an API change this significant, because as you said it could break a lot of peoples existing code. |
Hi, I worked on this issue for quite a while and would like to submit a PR. In order to make the "Add an entry in CHANGES.rst summarizing the change and linking to the issue" point of checklist done there are questions:
|
Yes to both. Thanks for working on this! |
I've got fiew more questions, perhaps they are a little bit silly, but nevertheless:)
|
I'm hesitant to allow disabling this. Perhaps if there is very clear documentation around "If you disable this on a command, it will be impossible for your users to provide an argument that looks like an option. Make sure this situation will never come up before doing this." The developer would also need to disable this individually for every command they do not want it on. Each command in the nested processing handles parsing individually, so each one might take an argument that could look like an option, and it's impossible for a command to know it's the last one until after its processing is complete. |
Hello, David. Glad to hear you, although we have war in Ukraine today, I will try to focus... |
What you implemented in your PR is a good solution for this issue, assuming that the issue should be addressed. I'm currently rewriting the parser, which is why I came back to this. Without discussing whether this particular CLI design is good, consider the following setup. @click.group()
@click.pass_context
@click.argument("path", type=click.Path())
def process(ctx, path):
click.ensure_object(dict)["path"] = path
@process.command()
@click.pass_context
@click.option("--save")
def reformat(ctx, save):
... Let's say you want to process a file whose name starts with a |
As the OP, I want to mention something. When I filed this, we had just written a CLI application with click at work. I was concerned about ways in which users could confuse themselves, and tried all sorts of stuff like playing with In the 5+ years since then, I haven't seen a single user get mixed up by this behavior. I think |
OK, based on the further discussion here, I'm going to close this. |
This is not a bug, but an enhancement request -- I'd like to be able to turn off this behavior.
Right now, Click allows a group name and a command name to be separated using the
--
separator.I think this is confusing, and I would like it to raise an error in my CLI.
To explain and provide an example, imagine a command group
foo
with a command in it namedbar
.You can write
and things are happy.
You can also write
and things work fine.
This behavior makes sense in my head, because
bar
is a positional argument tofoo
in a sense, but things are about to get messy and weird.This works, producing help output, and it really looks to me like it shouldn't. I guess the parsing for
bar
comes into play when it's read byfoo
and--
is just forgotten? I'm not sure.I would like to entirely disable the usage of
--
within the command tree. That is, I think it should be usable after a command name, but not between a group and a command.So,
foo -- ...
should be entirely disallowed because it's a group. The only reason to use--
is to pass things like--help
as arguments, and the parsing behavior here is confusing.I don't see a viable use-case for writing
foo -- bar ...
overfoo bar -- ...
.I could see this being added as an optional behavior, since making it the default will potentially break existing scripts.
In the meantime, is there some way that I can disable this for my own application in Click 5 or 6?
I'm also happy to write this and submit a PR, if I can get a little bit of direction on (1) where I should be looking to customize the parsing and (2) whether or not this is a feasible small change (vs. uprooting a lot of well-established code, which I'm somewhat averse to).
The text was updated successfully, but these errors were encountered: