Rework argument parsing in do/as providers #2813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current mechanism had a few oddities, as reported in erlang#2811: - `rebar3 do a,b` runs `a` then `b` - `rebar3 as profile task --a=b,c` runs the task with the flag `a` set to `b,c` - `rebar3 as profile task -a b,c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task - `rebar3 as profile task -a b, c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task So the issue is that to pass a comma to an argument, we can only do so when it exists as a long form argument (`--flag`) and that it does not contain spaces. This patch attempts to correct things by making the white-space following the comma significant. If it's missing, it gets grouped as a single entity. If it's there, the task is considered to be split. This becomes significant for commands that internally parse the `,` to allow list of args (such as `rebar3 release --relnames one,two`) which were only invokable as `relnames=one,two`, and not the spaced version nor the short version (`m one,two`), which should now be supported. This should *not* break any backwards compatibility, but there is the possibility that users who did invoke many commands with both arguments and sequences without spaces (`rebar3 do release --relname a,tar`) now get broken commands. I don't quite know if we'd be okay taking that risk, so this is up for comments I guess.
ferd
force-pushed
the
rework-arg-parsing-commas-in-do-as
branch
from
3f4d49b to
3051e66
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current mechanism had a few oddities, as reported in #2811:
rebar3 do a,brunsathenbrebar3 as profile task --a=b,cruns the task with the flagaset tob,crebar3 as profile task -a b,cruns the task with the flagaset tob, and then tries to runcas a taskrebar3 as profile task -a b, cruns the task with the flagaset tob, and then tries to runcas a taskSo the issue is that to pass a comma to an argument, we can only do so when it exists as a long form argument (
--flag) and that it does not contain spaces.This patch attempts to correct things by making the white-space following the comma significant. If it's missing, it gets grouped as a single entity. If it's there, the task is considered to be split.
This becomes significant for commands that internally parse the
,to allow list of args (such asrebar3 release --relnames one,two) which were only invokable as--relnames=one,two, and not the spaced version nor the short version (-m one,two), which should now be supported. Also do note that we seemingly deal with quoting fine, and sorebar3 do release --relnames "a, b"will prevent the whitespace from splitting the argument into commands.This should not break any backwards compatibility, but there is the possibility that users who did invoke many commands with both arguments and sequences without spaces (
rebar3 do release --relname a,tar) now get broken commands.I don't quite know if we'd be okay taking that risk, so this is up for comments I guess.