Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Added getter and setter of QuestionHelper to SymfonyStyle#17136
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
SCIF commentedDec 25, 2015
I did |
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.
could also be null
Tobion commentedDec 25, 2015
What do you need the getter for? |
ogizanagi commentedDec 26, 2015
IMO, this should be merged in 2.7 and should add SymfonyStyle tests for the questions rendering, as done for other methods inSymfonyStyleTest. As@Tobion said, I also think the getter won't be useful. However, even with this PR, commands using the |
SCIF commentedJan 14, 2016
@Tobion,here is one example of using getter. Getter required to allow passthrough input stream though different helpers if you want to replace ones.
It used by laravel framework for example (btw, as main helper, not in helperset). Symfony is component kit and should provide flexible features as i think. |
SCIF commentedJan 14, 2016
Fixed phpdoc. Thanks! |
xabbuh commentedJan 14, 2016
@SCIF The code you linked to doesn't make use of such a getter and I don't see why that would be necessary (the console helper set is something different). |
SCIF commentedJan 15, 2016
@xabbuh , that example shown that input stream sometimes required. If i want to replace QuestionHelper via setter — i would want to passtrough input stream hinted to original version of QuestionHelper to my own. It's not only testing purposes. Testing can be done without setter but substitution of QuestionHelper by different implementation can't be done without getter (to take user's input stream from it) and setter (to set new helper with hinted user's input stream). |
localheinz commentedJan 22, 2016
xabbuh commentedJan 22, 2016
@SCIF Imho then it makes more sense to let the |
SCIF commentedJan 22, 2016
@xabbuh , i really don't know why you think that getter of QuestionHelper is bad idea. But i can replace getter of Helper with InputStream getter if you insist. My arguments are above, you decide please something and it should be closed. @localheinz , thank you for your tests and work! Let i cherry pick commit with your tests and#17468 can be closed, isn't it? |
localheinz commentedJan 22, 2016
👍 If you like, go ahead! |
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 personally dislike the idea of creating a getter which can return either an object or null. It will introduce additional complexity above - the need of checking if the given result is an instance ofQuestionHelper
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.
Maybe just create aSymfonyQuestionHelper in__construct(),
sogetQuestionHelp() can always return an instance ofQuestionHelper
for example:
diff --git a/Style/SymfonyStyle.php b/Style/SymfonyStyle.phpindex 47d7d12..3402918 100644--- a/Style/SymfonyStyle.php+++ b/Style/SymfonyStyle.php@@ -47,6 +47,7 @@ class SymfonyStyle extends OutputStyle public function __construct(InputInterface $input, OutputInterface $output) { $this->input = $input;+ $this->questionHelper = new SymfonyQuestionHelper(); $this->bufferedOutput = new BufferedOutput($output->getVerbosity(), false, clone $output->getFormatter()); // Windows cmd wraps lines as soon as the terminal width is reached, whether there are following chars or not. $this->lineLength = min($this->getTerminalWidth() - (int) (DIRECTORY_SEPARATOR === '\\'), self::MAX_LINE_LENGTH);@@ -323,10 +324,6 @@ class SymfonyStyle extends OutputStyle $this->autoPrependBlock(); }- if (!$this->questionHelper) {- $this->questionHelper = new SymfonyQuestionHelper();- }- $answer = $this->questionHelper->ask($this->input, $this, $question); if ($this->input->isInteractive()) {@@ -356,6 +353,14 @@ class SymfonyStyle extends OutputStyle } /**+ * @return QuestionHelper+ */+ public function getQuestionHelper()+ {+ return $this->questionHelper;+ }++ /** * {@inheritdoc} */ public function newLine($count = 1)
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 don't feel like creating aSymfonyQuestionHelper object in the constructor is a good idea. There are many use cases where it's not used at all. Still,getQuestionHelper use case is very unclear for me, could you explain your point of view a little bit more please?
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 want to test the action of interactively input:
- creating a
SymfonyQuestionHelperobject in the constructor for making code simpler. - use
getQuestionHelper()to get theSymfonyQuestionHelperobject for test interactively input with mock viasetInputStream()
SCIF commentedMar 3, 2016
@javiereguiluz , which style getter do you want to be added by me? I askedthat but didn't get any answer. Just don't want to push fixes for |
sstok commentedMay 12, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Is there any chance of getting this merged? I'm currently using a hack to make my tests work, but I want to use a custom class to display a help text below the label and remove the "required" validator. |
Current SymfonyStyle implementation disallow to test interactively input of command accordingdocs.