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

Upgrade DialogHelper to QuestionHelper #3983

Merged
merged 13 commits into from Apr 30, 2015

Conversation

alcohol
Copy link
Member

@alcohol alcohol commented Apr 30, 2015

Closes #3760

* @param string $author
* @return array
*/
public function parseAuthorString($author)
Copy link
Contributor

Choose a reason for hiding this comment

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

public methods should stay before protected ones (and avoiding to move the code around like this also makes the review easier)

Copy link
Member

Choose a reason for hiding this comment

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

I agree on the review aspect, but it's kinda private anyway, it's only public because it has to be used by a closure, so in that case maybe tagging it with @private and moving it down makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I'm sorry. I'm just so used to putting helper methods after the config and execute methods. I'll revert that change.

Edit: marked it as @private instead.

@stof
Copy link
Contributor

stof commented Apr 30, 2015

This change currently requires bumping the Console requirement to 2.5+ (because it supports only the new API), and this is not possible yet because Packagist still runs on Symfony 2.3 and it needs Composer.
This means that the compatibility with Symfony Console 2.3 must be kept.

@Seldaek
Copy link
Member

Seldaek commented Apr 30, 2015

We already require 2.5, and I can live with that for now..

@alcohol
Copy link
Member Author

alcohol commented Apr 30, 2015

@Seldaek
Copy link
Member

Seldaek commented Apr 30, 2015

Thanks!

Seldaek added a commit that referenced this pull request Apr 30, 2015
Upgrade DialogHelper to QuestionHelper
@Seldaek Seldaek merged commit 8a12e50 into composer:master Apr 30, 2015
@alcohol alcohol deleted the upgrade-dialoghelper branch September 13, 2016 06:05
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.

Replace deprecated TableHelper and DialogHelper before updating Composer dependencies
4 participants