-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Finish 3744 #4405
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
Finish 3744 #4405
Conversation
👍 ty to have finished it. |
->setDefinition( | ||
new InputDefinition(array( | ||
new InputOption('foo', 'f'), | ||
new InputOption('bar', 'br', InputOption::VALUE_REQUIRED), |
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.
Have you tested this code? I believe shortcuts which are longer than 1 character will break things like -brbz
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 haven't. @mickaelandrieu did you test 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.
I tested it with single char codes (just what I showed in my comments in @mickaelandrieu's PR). I would recommend doing that
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.
Yeah, that probably better. I updated the 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.
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.
Than you @mickaelandrieu for sharing this. :)
|
||
Since the ``foo`` option doesn't accept a value, it will be either ``false`` | ||
(when it is not passed to the command) or ``true`` (when ``--foo`` was used | ||
by the user. The value of the ``bar`` option (and its ``b`` shortcut respectively) |
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.
missing )
after by the user
and maybe change "was used by the user" to "was set by the user" or "was passed"?
@wouterj I addressed your other comments. |
This looks great - thanks Christian and Mickaël! |
@@ -295,7 +295,7 @@ declare a one-letter shortcut that you can call with a single dash like | |||
.. tip:: | |||
|
|||
It is also possible to make an option *optionally* accept a value (so that | |||
``--yell`` or ``--yell=loud`` work). Options can also be configured to | |||
``--yell`` or ``--yell=loud`` or ``--yell loud`` work). Options can also be configured to |
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 "be --yell
, --yell=loud
or --yell loud
work"?
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.
Yea - we have too many or's :). Can you open up a quick PR? I'm on my phone now :)
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 created a PR -> #4691
This PR was merged into the 2.3 branch. Discussion ---------- replace "or" with "," See #4405 (comment) Just spelling. Commits ------- a950888 replace "or" with ","
This finishes the great pull request started by @mickaelandrieu in #3744.