Skip to content

[Console] Add a concept of 'tail' arguments #11757

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

Closed
wants to merge 1 commit into from

Conversation

damz
Copy link

@damz damz commented Aug 24, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This PR introduces the concept of a "tail argument". It is essentially an extension of the concept of InputArgument::IS_ARRAY that allows the last argument in the command line to unconditionally capture all the remaining arguments, whether they look like options or not.

The use case targeted here is the one of command lines that chains or wrap other command lines. It makes possible to implement those without forcing the user to use the -- separator to explicitly separate the options of the wrapper with the options of the wrapped command (at least when they are non-overlapping).

@damz damz changed the title Add an argument flag that captures all the remaining tokens. [Console] Add a concept of 'tail' arguments Aug 24, 2014
@damz damz force-pushed the pr/console-tail-arg branch from 9470533 to 1fc5c83 Compare August 25, 2014 07:48
@damz damz force-pushed the pr/console-tail-arg branch from 1fc5c83 to 1860379 Compare August 25, 2014 07:50
@fabpot
Copy link
Member

fabpot commented Aug 28, 2014

👍

throw new \InvalidArgumentException(sprintf('Argument mode "%s" is not valid.', $mode));
} elseif ($mode & self::IS_TAIL && !($mode & self::IS_ARRAY)) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this is bad DX. Adding the IS_TAIL flag should add IS_ARRAY automatically (by using 12 as value for the constant)

@stof
Copy link
Member

stof commented Aug 28, 2014

the issue with IS_TAIL is that this removes the possibility for the user to change the order of options.

for instance, if you just ran a command and want to see the same in verbose mode, you can move up in your history to get the command again, and then simply add -v at the end, and you get the verbose output (unless you explicitly used -- yourself, but you will see it).
If the tool is able to make the usage of -- implicit, this will break in unexpected way (it will simply add -v at the end of the array, which might do weird stuff in the command, and is unlikely to tell the user he is wrong about the place to put it)

@stof
Copy link
Member

stof commented Aug 28, 2014

Given these limitations, I think it might be OK to add it to cover the specific use case described here, but the documentation should clearly discourage using it in the general case

@@ -45,8 +46,10 @@ public function __construct($name, $mode = null, $description = '', $default = n
{
if (null === $mode) {
$mode = self::OPTIONAL;
} elseif (!is_int($mode) || $mode > 7 || $mode < 1) {
} elseif (!is_int($mode) || $mode > 15 || $mode < 1) {
Copy link
Member

Choose a reason for hiding this comment

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

@fabpot should we remove the upper bound check ? It makes it impossible to write forward-compatible commands (for instance a command trying to pass 12 as value to have IS_ARRAY | IS_TAIL for people using 2.6+ allowing to omit the --, but still being compatible with older Console versions to support the LTS (users would simply have to pass -- when wanting to pass an option).
If we decide to make the forward compatibility possible, we should probably do the change in 2.3 though (otherwise it would only allow it for 2.6+ for the next time we add a flag)

@fabpot fabpot added the Console label Sep 5, 2014
@nicolas-grekas
Copy link
Member

Do we still need this feature now that #11431 has been merged and released in 3.0?

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

I think we can close here.

@damz Do you still see anything left that cannot be achieved with the changes done in #11431?

@damz
Copy link
Author

damz commented Mar 7, 2016

The user experience is different, here I was trying to achieve this without the explicit -- marker. But functionally, everything we can do here is covered by #11431, so let's close this one.

@damz damz closed this Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants