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

Support a target separator (+) in the argument parser #1521

Merged
merged 6 commits into from
Oct 11, 2021

Conversation

lefou
Copy link
Member

@lefou lefou commented Oct 10, 2021

This PR adds support to specify arbitrary targets and target arguments in one mill run.

Motivation: Current mill only supports the selection of one or multiple targets followed by an argument list which is applied to all targets. Providing different arguments is currently not supported.

Solution: We introduce a new separator + which separates the target arguments from the next target. To pass the + as argument to a target, masking with \+ is supported.

Example:

mill __.compile + core.testCached + itest.test fastTest slowTests + __.publishLocal

This command will:

  • run compile for all modules
  • run testCached for module core
  • run itest.test command with arguments fastTest and slowTests
  • run publishLocal for all modules

The limitation, that a command can only run once is still present and not changed by this PR.

I could imagine, that we later also provide a ++ separator, to specify targets that need to run after each other. This would also allow running the same command multiple times (with different args).

This also makes the all and par commands obsolete.

Review by @lihaoyi

@lefou lefou changed the title Support special separator argument parser Support a target separator in the argument parser Oct 10, 2021
@lefou lefou changed the title Support a target separator in the argument parser Support a target separator (+) in the argument parser Oct 10, 2021
@lihaoyi
Copy link
Member

lihaoyi commented Oct 11, 2021

@lefou looks great. I like the idea of keeping the separate tokens separate; originally I had thought about putting each set of command + arguments into a single string/token like SBT does, but that opens up a big can of worms: nested escaping, tokenization, etc. Your idea is better

What do you think of using -- instead of + as the separator? I'm a bit wary of using unusual punctuation characters, as they tend to be handled in odd ways by different shells. e.g. square brackets for cross builds need to be quoted when using zsh, which every OSX user now defaults to. -- is pretty common, and since the old way of separating arguments is going away we can repurpose it. That would mean we can never pass -- to a mill command itself, but I suppose that's not too different from being unable to pass + to a mill command.

Just a thought, either way what you propose is far better than what we currently havr

@lefou
Copy link
Member Author

lefou commented Oct 11, 2021

@lefou looks great. I like the idea of keeping the separate tokens separate; originally I had thought about putting each set of command + arguments into a single string/token like SBT does, but that opens up a big can of worms: nested escaping, tokenization, etc. Your idea is better

Glad you like it.

What do you think of using -- instead of + as the separator? I'm a bit wary of using unusual punctuation characters, as they tend to be handled in odd ways by different shells. e.g. square brackets for cross builds need to be quoted when using zsh, which every OSX user now defaults to. -- is pretty common, and since the old way of separating arguments is going away we can repurpose it. That would mean we can never pass -- to a mill command itself, but I suppose that's not too different from being unable to pass + to a mill command.

TLDR: I'd like to not use -- (it's already taken), and the best other symbol is IMHO +.

Having a rather long Linux background, I'm used to this one single true meaning of --: stop parsing arguments! If we would use it as separator we would invalidate this, as it can occur multiple times and it does not guarantee to stop parsing arguments. At least not for the used commands (which might be embedded tools). Also in current mill we already have a meaning for --, which is: separate target and their arguments, in multi-mode. So, there is even a greater chance of confusion and the risk that older examples/user scripts won't work anymore.

I have thought rather long about a good separator, and from my experience + is rather safe in all shells. It is already used by some tools to start (negated) short options, e.g. set +x to disable set -x. And as we support masking/escaping, we can still pass a + as argument by using \+. Also the semantic of + is very intuitive: add. That's what we do, add more tasks/commands.

@lihaoyi It would be great if you could double check, that the + works as expected in zsh on OSX?

@lihaoyi
Copy link
Member

lihaoyi commented Oct 11, 2021

Good point about +x. Given how common that is, I doubt anyone would be willing to break it. I don't have my laptop on me but you've convinced me + should be fine.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 11, 2021

How thorough is the masking? e.g. if i want to pass a literal \+, should I do \\+? Does it apply to +s embedded in bigger strings, or only standalone?

@lefou
Copy link
Member Author

lefou commented Oct 11, 2021

How thorough is the masking? e.g. if i want to pass a literal \+, should I do \\+?

Good point. I'll add a test for that.

@lefou
Copy link
Member Author

lefou commented Oct 11, 2021

Only a standalone + optionally preceded by any number of masking symbols (\) are handled. In case there are masking symbols, exactly one is removed.

@lefou lefou merged commit 1b02955 into com-lihaoyi:main Oct 11, 2021
@lefou lefou deleted the target-parser branch October 11, 2021 15:28
@lefou lefou added this to the after 0.10.0-M3 milestone Oct 11, 2021
lefou added a commit to lefou/mill that referenced this pull request Nov 12, 2021
This adds support to specify arbitrary targets and target arguments in one mill run.

__Motivation:__ Current mill only supports the selection of one or multiple targets followed by an argument list which is applied to all targets. Providing different arguments is currently not supported.

__Solution:__ We introduce a new separator `+` which separates the target arguments from the next target. To pass the `+` as argument to a target, masking with `\+` is supported.

Example:

```bash
mill __.compile + core.testCached + itest.test fastTest slowTests + __.publishLocal
```

This command will:
* run `compile` for all modules
* run `testCached` for module `core`
* run `itest.test` command with arguments ` fastTest` and `slowTests`
* run `publishLocal` for all modules

The limitation, that a command can only run once is still present and not changed by this PR.

See pull request: com-lihaoyi#1521
lefou added a commit to lefou/mill that referenced this pull request Nov 12, 2021
This adds support to specify arbitrary targets and target arguments in one mill run.

__Motivation:__ Current mill only supports the selection of one or multiple targets followed by an argument list which is applied to all targets. Providing different arguments is currently not supported.

__Solution:__ We introduce a new separator `+` which separates the target arguments from the next target. To pass the `+` as argument to a target, masking with `\+` is supported.

Example:

```bash
mill __.compile + core.testCached + itest.test fastTest slowTests + __.publishLocal
```

This command will:
* run `compile` for all modules
* run `testCached` for module `core`
* run `itest.test` command with arguments ` fastTest` and `slowTests`
* run `publishLocal` for all modules

The limitation, that a command can only run once is still present and not changed by this PR.

See pull request: com-lihaoyi#1521
lefou added a commit that referenced this pull request Nov 13, 2021
#1566)

This adds support to specify arbitrary targets and target arguments in one mill run.

__Motivation:__ Current mill only supports the selection of one or multiple targets followed by an argument list which is applied to all targets. Providing different arguments is currently not supported.

__Solution:__ We introduce a new separator `+` which separates the target arguments from the next target. To pass the `+` as argument to a target, masking with `\+` is supported.

Example:

```bash
mill __.compile + core.testCached + itest.test fastTest slowTests + __.publishLocal
```

This command will:
* run `compile` for all modules
* run `testCached` for module `core`
* run `itest.test` command with arguments ` fastTest` and `slowTests`
* run `publishLocal` for all modules

The limitation, that a command can only run once is still present and not changed by this PR.

See pull request: #1521

Pull request: #1566
lefou added a commit to lefou/mill that referenced this pull request Jan 4, 2022
Fixes com-lihaoyi#1648

This issue was introduced in com-lihaoyi#1648
which is a backport of com-lihaoyi#1521
It was probably introduced while resolving merge conflicts and
is not present in the main branch.
lefou added a commit that referenced this pull request Jan 4, 2022
Fixes #1648

This issue was introduced in #1648
which is a backport of #1521
It was probably introduced while resolving merge conflicts and
is not present in the main branch.

Pull request: #1649
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants