-
-
Notifications
You must be signed in to change notification settings - Fork 674
A new "pants next-gen" command-line parser. #22808
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
Conversation
This supports the command/subcommand paradigm, rather than the "list of goals" paradigm of the existing cli parser. This is experimental, not wired up to anything yet, and intended to support a "next-generation Pants" (pants_ng for short). It is not intended to support all current Pants 2 use-cases, but is a much simpler design based on our experience with the complexities of the current cli parser. In particular, this new cli parser does not need to know about any registered options (unlike the old one, which uses them to tell the difference between specs and goals). This parser instead requires specs to contain a filepath separator. This cli parser currently supports one cmd (and optionally one subcommand) per invocation. If we want to support multiple cmd/subcommand pairs in a single invocation in the future we can do so by using a delimiter that isn't interpreted by the shell. See pantsbuild#22692 for context.
|
Haven't reviewed this yet (trying to get that stupid JS update landed first), but one idea would be to plan for bash/zsh completions early on. I think several of the clap/clap-derivatives have that built-in. Better than trying to tack it on. The one we have in mainline, that I intend to update in a few PRs, is finally "good enough" because of how fast the call-by-name made running everything. Previously, it was brutal to have to wait. However, nothing will beat auto-generated out of the box (not necessarily this PR though) |
To be clear, this just parses CLIs, but has no knowledge at all of what commands or flags are available. That would be a separate thing, which (unlike this parser) is aware of every backend. And which I agree should plan for completions. |
cognifloyd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm bike shedding here. I don't intend to nitpick or be mean or whatever. Just thinking through what it would be like to use this.
Thank you for working on this. And _ng seems like a fine non-committal suffix.
| // cmd1 --cmd1-flags (subcmd1 --subcmd1-flags)? \ | ||
| // @ cmd2 --cmd2-flags (subcmd2 --subcmd2-flags)? \ | ||
| // ... \ | ||
| // @ cmdN --cmdN-flags (subcmdN --subcmdN-flags)? \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be odd here, but I like to "read" the commands I type replacing symbols and single characters with a word or functional description. When I write s/this/that/ in a sed command, instead of "s" I mentally say "switch this with that".
So, the @ reads oddly to me: "pants cmd1 ... (AT) cmd2 ..." sounds like cmd1 is a subcmd of cmd2. I'm not saying I want something different, but I would like to understand your thinking better here.
Other potential separators:
+
like "and" without using "&"
"pants cmd1 ... + cmd2 ..." reads as "(do) cmd1 ... (AND) cmd2 ..."^
like regex "start" of line or git "parent commit"
"pants cmd1 ... ^ cmd2 ..." reads as "(start) cmd1 ... (start) cmd2 ..."';'(like xargs)
I don't actually like the xargs interface, but it is interesting prior art.-and(like find)
This is probably too verbose. Still interesting to consider.
I suppose I could get used to using @ as the separator. I would just read this as "at global" or "at the top" or something like that.
What led you to select @?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the first character I thought of that isn't interpreted by the shell... So semicolon is out. But I agree that + would be more evocative, so I'm down with that.
| ); | ||
|
|
||
| assert_eq!( | ||
| mk_invocation("pants --foo cmd --bar=baz").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could use a special character at the beginning of a flag to say "this is a global flag".
Maybe , the @ would be a good "global flag" marker. So people (like me) can through a global flag (like keep-sandboxes on the end of a command from shell history like this
pants --foo CMD --bar=baz --@keep-sandboxes=always
Or ^:
pants --foo CMD --bar=baz --^keep-sandboxes=always
Clearly, that could only work before any passthrough args (before --).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we could also say that any flags after specs are global (instead of an error), without any special syntax? There is nothing else they can be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the idea is to be able to tack global flags on at the end of an existing cmd line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the idea is to be able to tack global flags on at the end of an existing cmd line?
Yeah. Some debug or log level commands on the end of a failed command can be really helpful for me.
Either going up in shell history or copy/paste from CI and then tack on the debug arg(s) (which is often --keep-sandboxes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we could also say that any flags after specs are global (instead of an error), without any special syntax? There is nothing else they can be.
Huh. I suppose that would work. Cool. I like it! Would anyone find that confusing or surprising?
Your comments are valuable and I don't see them as nitpicking, and I certainly don't detect meanness, so don't worry about it. |
| let mut passthru = None; | ||
| if let Some(s) = unconsumed_args.next() { | ||
| if s == "--" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling global args after specs would mean adding a consume_flags call right before this passthrough arg handling, right?
Should something like that go in a follow-up (if we do it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, it was easy enough to do so I've done it.
cburroughs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together.
This parser instead requires specs to contain a filepath separator,
anything else is a flag or a command or a subcommand, and those
are easily distinguished by syntax alone. A spec path in the root dir
can be prefixed with ./ to meet this requirement.
So instead of src::, it would be something like ./src::?
| #[test] | ||
| fn test_multiple_commands_and_subcommands() { | ||
| assert_eq!( | ||
| mk_invocation("pants cmd1 subcmd1 + cmd2 + cmd3 subcmd3").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So obviously | as a character is out, but I'm wondering if these should be thought of as "do all of this and then do the next one", or "maybe 'line' by 'line' depending on the command".
Thinking of #10542 for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Different characters for sequential and concurrent goals? I was thinking only of sequential goals like the current interface. Adding the possibility of concurrency or "goal scripting" opens a whole new works of possibilities. 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best if these were always concurrent, no? You can get sequential behavior by running pants multiple times.
| ); | ||
|
|
||
| assert_eq!( | ||
| mk_invocation("pants --global-flag cmd1 --cmd1-flag subcmd1 --subcmd1-flag + cmd2 --cmd2-flag + cmd3 --cmd3-flag subcmd3 --subcmd3-flag path/to/spec -- passthru").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--cmd1-flag should be read as "an example flag used by --cmd1, not a required prefix, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
Exactly |
cognifloyd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start. Looking forward to where this might go.
|
|
||
| #[inline] | ||
| fn is_spec(s: &str) -> bool { | ||
| s.contains(path::MAIN_SEPARATOR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know rust. When we support Windows at some point, will this be \ on Windows instead of /? Or will it take both? Or does it always use the *nix /?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be backslash on Windows, yep: https://doc.rust-lang.org/std/path/constant.MAIN_SEPARATOR.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support both on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not at first. Maybe if there is demand for it. But we are a thousand other steps away from even evaluating that demand...
This supports the command/subcommand paradigm, rather than
the "list of goals" paradigm of the existing cli parser.
This is experimental, not wired up to anything yet, and intended to
support a hypothetical "next-generation Pants" (pants_ng for short).
It is not intended to replicate all current Pants CLI nuances, but is a
much simpler design based on our experience with the complexities
of the current CLI parser.
In particular, this new CLI parser does not need to know in advance
about any registered goals and options. The old parser needed this
to tell the difference between specs and goals, and this added a
lot of complexity for little gain. This was also ambiguous, since
introduction of a new goal could change the meaning of an existing
invocation.
This parser instead requires specs to contain a filepath separator,
anything else is a flag or a command or a subcommand, and those
are easily distinguished by syntax alone. A spec path in the root dir
can be prefixed with
./to meet this requirement.This parser supports multiple cmd/subcommand pairs, each with
flags, by separating them with a standalone
+character. This,again, removes ambiguity.
This parser supports only long flags (prefixed by
--). We currentlysupport a handful of short flags (such as
-l), but those causeambiguity since we also use a single dash prefix for negating
specs.
See #22692 for context.