Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 7, 2014
Merged

Finish 3744 #4405

merged 3 commits into from
Dec 7, 2014

Conversation

@xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 1, 2014

This finishes the great pull request started by @mickaelandrieu in #3744.
@xabbuh xabbuh mentioned this pull request Nov 1, 2014
Closed
@mickaelandrieu
Copy link
Contributor

👍 ty to have finished it.
wouterj
wouterj reviewed Nov 1, 2014
View reviewed changes
components/console/console_arguments.rst Outdated
Copy link
Member

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
Copy link
Member Author

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?
Copy link
Member

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
Copy link
Member Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better as I didn't remember if I've tested this sample, thanks for the tip @wouterj,
and thanks (again) @xabbuh to finish this: console arguments docs realy need this add :)
Copy link
Member Author

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. :)
wouterj
wouterj reviewed Nov 5, 2014
View reviewed changes
components/console/console_arguments.rst Outdated
Copy link
Member

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"?
@xabbuh
Copy link
Member Author

xabbuh commented Nov 7, 2014

@wouterj I addressed your other comments.
@weaverryan
Copy link
Member

This looks great - thanks Christian and Mickaël!
@weaverryan weaverryan merged commit 1ce59ee into symfony:2.3 Dec 7, 2014
weaverryan added a commit that referenced this pull request Dec 7, 2014
@weaverryan
feature #4405 Finish 3744 (mickaelandrieu, xabbuh)
39a36bc 
This PR was merged into the 2.3 branch. Discussion ---------- Finish 3744 This finishes the great pull request started by @mickaelandrieu in #3744. Commits ------- 1ce59ee finish #3744 f503596 [WIP] - Console add Console arguments page d2b69b6 Added a little sample on Option uses with "spaces"
@xabbuh xabbuh deleted the finish-3744 branch December 7, 2014 22:25
timglabisch
timglabisch reviewed Dec 27, 2014
View reviewed changes
components/console/introduction.rst
Copy link
Contributor

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"?
Copy link
Member

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 :)
Copy link
Contributor

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
@timglabisch timglabisch mentioned this pull request Dec 27, 2014
Merged
wouterj added a commit that referenced this pull request Dec 28, 2014
@wouterj
minor #4691 replace "or" with "," (timglabisch)
8d32393 
This PR was merged into the 2.3 branch. Discussion ---------- replace "or" with "," See #4405 (comment) Just spelling. Commits ------- a950888 replace "or" with ","
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Reviewed

5 participants

@xabbuh @mickaelandrieu @weaverryan @timglabisch @wouterj