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

[Console] Add callback support to Console\Question autocompleter #30997

Merged
merged 1 commit into from Apr 8, 2019
Merged

[Console] Add callback support to Console\Question autocompleter #30997

merged 1 commit into from Apr 8, 2019

Conversation

MikkelPaulson
Copy link
Contributor

@MikkelPaulson MikkelPaulson commented Apr 8, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? minor edge case, see below
Deprecations? no
Tests pass? yes (with expanded coverage)
Fixed tickets N/A
License MIT
Doc PR symfony/symfony-docs#11349

Autocompletion is a useful feature, but it's not always possible to anticipate every input the user could provide in advance. For instance, if we're allowing the user to input a path to a file, it's not practical to populate an array with every file and directory in the filesystem, but we can easily build a callback function that populates its suggestions based on the path already inputted.

This change replaces the autocomplete logic that accepts an array of suggestions with an architecture that uses a callback function to populate suggestions in real time as the user provides input.

The first commit adds a test class covering all methods of the Question object, while the second commit modifies the Question object to accept and store a callback function. The existing [gs]etAutocompleterValues() methods are preserved, but instead of being referenced directly from the QuestionHelper, they create and call their own callbacks to emulate the current behaviour.

There is one edge case that is changed, as documented in the test: when a Traversable object is passed to setAutocompleterValues(), the return value of getAutocompleterValues() will be the unpacked (array) form of that object rather than the object itself. The unpacking is done lazily and cached on the callback function.

@MikkelPaulson
Copy link
Contributor Author

The CI failures on this branch also appear to be occurring on the upstream master...

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM. I've left some comments about CS.

src/Symfony/Component/Console/Helper/QuestionHelper.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Question/Question.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 8, 2019
@nicolas-grekas nicolas-grekas changed the title Add callback support to Console\Question autocompleter [Console] Add callback support to Console\Question autocompleter Apr 8, 2019
In order to enable more dynamic use cases such as word-by-word
autocomplete and path-based autocomplete, update the autocomplete logic
of the Question object and its helper to accept a callback function.
This function is called on each keystroke and should return an array of
possibilities to present to the user.

The original logic only accepted an array, which required
implementations to anticipate in advance all possible input values.

This change is fully backwards-compatible, but reimplements the old
behaviour by initializing a "dumb" callback function that always returns
the same array regardless of input.
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I addressed Fabien's comment + did some changes while reviewing)

@fabpot
Copy link
Member

fabpot commented Apr 8, 2019

Thank you @MikkelPaulson.

@fabpot fabpot merged commit caad562 into symfony:master Apr 8, 2019
fabpot added a commit that referenced this pull request Apr 8, 2019
…ocompleter (Mikkel Paulson)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Console] Add callback support to Console\Question autocompleter

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | minor edge case, see below
| Deprecations? | no
| Tests pass?   | yes (with expanded coverage)
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | symfony/symfony-docs#11349

Autocompletion is a useful feature, but it's not always possible to anticipate every input the user could provide in advance. For instance, if we're allowing the user to input a path to a file, it's not practical to populate an array with every file and directory in the filesystem, but we can easily build a callback function that populates its suggestions based on the path already inputted.

This change replaces the autocomplete logic that accepts an array of suggestions with an architecture that uses a callback function to populate suggestions in real time as the user provides input.

The first commit adds a test class covering all methods of the `Question` object, while the second commit modifies the `Question` object to accept and store a callback function. The existing `[gs]etAutocompleterValues()` methods are preserved, but instead of being referenced directly from the `QuestionHelper`, they create and call their own callbacks to emulate the current behaviour.

There is one edge case that is changed, as documented in the test: when a `Traversable` object is passed to `setAutocompleterValues()`, the return value of `getAutocompleterValues()` will be the unpacked (array) form of that object rather than the object itself. The unpacking is done lazily and cached on the callback function.

Commits
-------

caad562 [Console] Add callback support to Console\Question autocompleter
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 9, 2019
This PR was merged into the master branch.

Discussion
----------

Add description of autocompleter callback

Documented feature added by symfony/symfony#30997.

Commits
-------

5cfc67e Add description of autocompleter callback
fabpot added a commit that referenced this pull request Apr 10, 2019
This PR was merged into the 4.3-dev branch.

Discussion
----------

Improve test coverage from #30997

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes (test enhancement against change on master)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | enhancement for #30997
| License       | MIT
| Doc PR        | N/A

Test coverage added in #30997 did a good job of validating previous behaviour, but didn't adequately cover the new callback logic. Added coverage for new methods on the Question object.

Commits
-------

4693422 Improve test coverage from #30997
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants