-
Notifications
You must be signed in to change notification settings - Fork 87
Convert questions to a string, fixes #243 #244
Convert questions to a string, fixes #243 #244
Conversation
Since symfony/console 4 questions only accept a question string. Looking at its history, this was always the case and is wrong in the composer docblock. see: https://github.com/symfony/console/blob/4.0/Question/Question.php#L37 see: https://github.com/composer/composer/blob/1.6/src/Composer/IO/IOInterface.php#L106
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.
LGTM 👍
@xtreamwayz It's wrong milestone, should be 3.0.2.
3.0.1 is already released.
'Questions must be a string since symfony/console:4.0' | ||
); | ||
|
||
self::assertThat( |
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.
Why not assertTrue
?
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.
Good question.
@weierophinney any reason you wrote it like this?
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 think you want to ask @weierophinney ;-)
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.
Works for me :)
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.
Honestly, because it's how custom assertions are demonstrated in the manual: https://phpunit.de/manual/current/en/extending-phpunit.html
I'm fine with either approach.
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.
Hm... in the docs is just example, maybe a bit unfortunate, because assertTrue
is one of the available assertions.
I just noticed when creating a new project :) |
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.
Looks good, and I think we may need to do similar in the zend-component-installer.
'Questions must be a string since symfony/console:4.0' | ||
); | ||
|
||
self::assertThat( |
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.
Honestly, because it's how custom assertions are demonstrated in the manual: https://phpunit.de/manual/current/en/extending-phpunit.html
I'm fine with either approach.
Working on it. |
Thanks, @xtreamwayz! |
Since symfony/console 4 questions only accept a question string. Looking at its history, this was always the case and is wrong in the composer docblock.
see: https://github.com/symfony/console/blob/4.0/Question/Question.php#L37
see: https://github.com/composer/composer/blob/1.6/src/Composer/IO/IOInterface.php#L106