Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Console] Added better interaction for choice questions#11326

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

Closed
florianv wants to merge2 commits intosymfony:masterfromflorianv:choice-question

Conversation

@florianv
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

This PR adds a more user friendly way to ask choice questions whenstty is available.

Single choice question

single

You can move up and down with the arrow keys.
To validate your choice pressenter.

Multi choice question

multi

You can move up and down with the arrow keys.
To select / deselect a choice press thespacebar.
To validate your choices pressenter.

Todos:

  • Finish tests
  • Documentation

nervo, chalasr, and stephanvierkant reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

i think putting @api on the class is enough. I mean I would assume it would have methods unshakable

Copy link
Member

Choose a reason for hiding this comment

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

Actually,@api shoulmd never be added in a PR submitting the code. you cannot make it part of the stable API if it is not even part of the codebase.
We will mark it as part of the stable API only a bit later

@florianv
Copy link
ContributorAuthor

I was thinking that this helper could replace the current one for choice questions only whenstty is available. This could be done by renamingQuestionHelper intoGenericQuestionHelper for example, and then creating a newQuestionHelper that just proxies the other two:

publicfunctionask(InputInterface$input,OutputInterface$output,Question$question){if ($this->hasSttyAvailable() &&$questioninstanceof ChoiceQuestion) {return$this->helperSet->get('choice_question')           ->ask($input,$output,$question);   }return$this->helperSet->get('generic_question')       ->ask($input,$output,$question);}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Note that here I pull the cursor left of 10 000 columns to avoid usingmb_strlen (maybe less should be enough too).

@stof
Copy link
Member

stof commentedJul 8, 2014

IMO, it does not mak sense to have a separate helper for choice questions. It makes things harder to use for developers, by requiring to choose the right helper (or they will always use thequestion helper they already know, and users will not benefit from the better experience).
The enhanced experience should be implemented in the QuestionHelper directly (using the existing logic when you don't have stty)

@florianv
Copy link
ContributorAuthor

I didn't put it in theQuestionHelper because I feared it becomes a mess with the different handlings (for example they write the questions and choices differently). On the other side the choice question handling is mostly done by theSingleChoiceQuestionAsker and theMultiChoiceQuestionAsker.

What I could do is to put the default handling fromhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Helper/QuestionHelper.php#L110 in a separate command likeGenericQuestionAsker to keep theQuestionHelper clean. It will choose the appropriateAsker depending on the type of question and ifstty is available and use that directly.

What do you think@stof ?

@florianvflorianv changed the title[Console] Added a new helper for choice questions[Console] Added better interaction for choice questionsJul 11, 2014
@florianv
Copy link
ContributorAuthor

I have updated the PR, so it keeps only theQuestionHelper and uses the new way whenstty is available and it's a choice question. I have also completed the tests.

@florianv
Copy link
ContributorAuthor

Btw, it would be nice if someone withoutstty could run the tests.

@florianv
Copy link
ContributorAuthor

What do you think about this PR ?@fabpot@stof

Copy link
Member

Choose a reason for hiding this comment

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

I don't like these options. They are making the helper stateful, which is causing issues (it means asking a question in one place will have side effects on next questions in other places).

The configuration should be part of the Question object instead

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes but I think when you want to configure the templates, you want to do it globally for all questions.

Copy link
Member

Choose a reason for hiding this comment

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

but here, your options are mutable. This means that some code using the helper can decide to change the options, and this will impact unrelated code (which was expecting to use the defaults).

If the goal is to configure them globally, they should not be mutable by code using the helper

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it's better to attach them to the Question as you propose. Thanks

@eko
Copy link
Contributor

eko commentedJul 17, 2014

This is a great addition, a much better way to give answers, user-friendly 👍

@mickaelandrieu
Copy link
Contributor

👍

@florianv
Copy link
ContributorAuthor

Thanks for reviewing@stof, I have addressed your comments

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure these utility classes/Interface should actually be in the root of the component (I don't have a good idea for the naming of a subnamespace holding them though).

also, Symfony uses a final class with a private constructor for enums, not an interface (it would not make sense for someone to implement this interface)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

MaybeIOUtil or something like that ?

@jjsaunier
Copy link

👍

@florianv
Copy link
ContributorAuthor

Any new about this PR@fabpot@stof ?

@fabpot
Copy link
Member

@ro0NL Probably one that you could be interested in taking over :)

@vudaltsov
Copy link
Contributor

I guess, this PR could be closed after#26698.

@fabpotfabpot closed thisFeb 3, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrAwaiting requested review from chalasr

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

12 participants

@florianv@stof@eko@mickaelandrieu@jjsaunier@fabpot@vudaltsov@cordoval@javiereguiluz@romainneutron@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp