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

Allow short arguments and flags to be combined #102

Merged
merged 8 commits into from
Jan 26, 2024
Merged

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Jan 25, 2024

Fixes #7

This PR allows flags like -abtrue to mean -a -b true, by walking the combined flag one character at a time and seeing if each character is a flag that takes zero values, or a non-flag argument that takes one value, which is set to the remaining part of the combined flag (in the case above, "true"). This allows some nice shorthands, like ./mill -wk -j10 rather than ./mill -w -k -j 10

This is the only way I could find that allows both combining multiple flags like -ab, and combining flags with values like -btrue. Trying to handle more sophisticated cases like tar tfv <file> where f is given <file> is fundamentally incompatible, and would need some user-facing configuration to enable.

Combining multiple short arguments together with gflags = syntax, e.g. -ab=true, is currently not allowed. This follows the current limitation where we do not allow single short args to be used with =, e.g. -f=bar is prohibited. In general, combined short arguments are not allowed to have = in it. If you want the = to be part of the value, you can move it into a separate token e.g. -ab =true

For now, this improvement can be done fully transparently and backwards-compatibly without any need for user opt-in or configuration, and should cover the 99% use case. There is no conflict with existing long arguments, as those currently require two dashes --, and so a multi-character token starting with a single - is currently disallowed. Adding additional flexibility and configuration can come later future

Covered by additional unit tests

@lihaoyi lihaoyi requested review from lefou and lolgab January 25, 2024 05:04
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature. And since we preserved the binary compatibility, we can use it in Mill right away. I'm looking forward for working mill -Denv=var support (without the space after -D), which popped up in the support channels more than once.

@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 25, 2024

@lefou as implemented, -Denv=var won't work, due to the limitations written in the PR description.

I think we can make it work by specifying that -abc=xyz parses as -a bc=xyz. It may be a bit ambiguous to humans, because it could easily look like -ab c=xyz or -abc =xyz, but from a technical point of view it is unambiguous.

Do you think we should do that? Should be an easy change if we want it

@lefou
Copy link
Member

lefou commented Jan 25, 2024

Oh, than I misread it. What's the difference between -abtrue and -aDenv=var then? -b accepts one arg of type Boolean, -D accepts one arg of type String.

Anyways, I find properties like being able to combine short no-arg options (-dk) and being able to provide a single arg to a short option (-j0, -Denv=val) more important to support, than having both features but with limitations.

@lefou
Copy link
Member

lefou commented Jan 25, 2024

How about -ab true or -a -btrue? If the first short option accepts an arg, we do not accept more short option without a space. If not, than all accepting args must come after an space.

So either -a -Denv=val or -aD env=val.

@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 25, 2024

I updated the PR to allow -Denv=value to mean -D env=value.

As specified, it also means -aDenv=value means -a Denv=value. Not sure if this would be surprising, but I might be able to make it mean -a -D env=value with a bit more fiddling

@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 25, 2024

@lefou ok I think I got it working. take a look at the update to readme.md to see if it makes sense.

I think this handles most cases in a reasonably intuitive manner; the only bit of weirdness is that -j10 and -j=10 mean the same thing, and the leading = in the value is skipped. But I think that's probably alright given what a user would typically intend when writing out those tokens (and they can always write -j =10 if they want to explicitly pass in a value with a leading =)

@lihaoyi lihaoyi merged commit d7b8eaf into main Jan 26, 2024
4 checks passed
@lihaoyi lihaoyi deleted the combined-short-args branch January 26, 2024 01:03
@lefou lefou added this to the 0.6.0 milestone Jan 26, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Jan 26, 2024

Notably, our treatment of combined multiple-short-flags-and-argument-values matches some existing tools like Git:

$ git commit -am.
[test f25b226a55] .

$ git commit -ma. --allow-empty
[test d71b21942f] a.

Note how -am. treats a as a separate flag and . as the value, but -ma. treats a. as the combined value due to -m being a value-taking argument. This matches the semantics I have chosen for MainArgs

lihaoyi added a commit that referenced this pull request Sep 13, 2024
This was accidentally broken in
#102 and
#112, and wasn't covered by
tests. Noticed when trying to update Ammonite to the latest version of
MainArgs in com-lihaoyi/Ammonite#1549

Restored the special casing for tracking/handling incomplete arguments
and added some unit test cases
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.

short args should be parsed when there is no space after
2 participants