Feature: input parameters (promptable options in interactive mode) #20
Conversation
Signed-off-by: Michał Bundyra <contact@webimpress.com>
Signed-off-by: Michał Bundyra <contact@webimpress.com> Co-authored-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Michał Bundyra <contact@webimpress.com>
Signed-off-by: Michał Bundyra <contact@webimpress.com>
…type Signed-off-by: Michał Bundyra <contact@webimpress.com>
There was a problem hiding this comment.
@weierophinney Thanks for your review. You haven't said anything what you think about this feature :) i've applied some suggestions and added some comments with clarification.
This implementation was the easiest I could think of in terms of integration.
It just requires adding the trait and all works very smooth with laminas-cli.
Changing current commands which are using options is trivial (see my changes in laminas-mvc).
I've found also this library: https://packagist.org/packages/vivait/symfony-console-promptable-options which serves similar functionality. The problem there is that we have to "initialize" promptable options in execute, and I wanted to eliminate this additional step.
I've tried also use helper (as we have for example QuestionHelper, but I had very similar problem there - how to inject input/output there.
docs/book/command-params.md
Outdated
| [ | ||
| 'type' => 'dir', // 'dir', 'file' or null (for both) | ||
| 'existing' => true, // if path must exist, default false | ||
| // @todo not supported yet, not sure if needed: |
There was a problem hiding this comment.
The idea here was that we define the base path, so the autosuggestion wouldn't work outside, and value which is outside of this directory will be invalid. I am not sure if it is really needed, tho.
Oh, I like it! I think we can do a little more to finesse it, but the idea of turning options into prompts could simplify a TON of workflows. |
This patch introduces the interface `InputParamInterface`, defining the various methods every input parameter must define. It also introduces two traits to make defining parameters easier: - InputParamMethodsTrait (defines most methods) - ValidatedQuestionTrait (extends InputParamMethodsTrait to allow adding a validatable question) From there, it introduces discrete parameter types: - BoolParam - ChoiceParam - IntParam - PathParam - StringParam Each inlines their various validators, and provides methods for configuring them directly on the input; this allows us to make the validation options typesafe, and exposes them more easily to consumers. The `InputParamTrait` was updated such that `addParam()` now accepts an `InputParamInterface` instance, and pulls information from it in order to create an `InputOption` for the application. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
This patch creates a custom symfony/console `InputInterface`
implementation, `ParamAwareInput`, which decorates an existing
`InputInterface` implementation, and adds the method `getParam()` to it.
The proxy class also wraps over:
- The output
- The current question helper instance
- Parameters from the current command
When `getParam()` is called, it will attempt to retrieve the associated
option, returning the value if found. Otherwise, it will prompt using
the question associated with the parameter, the question helper in the
proxy, and the output from the proxy.
I have updated `InputParamTrait` to add a `run()` method; it calls on
the parent `run()` method, but by first decorating the provided `$input`
in a `ParamAwareInput`.
This approach allows removal of the custom `Application` instance
entirely, and makes consumption of the new input parameters more in line
with options and arguments, in that you now call:
```php
$value = $input->getParam('some-name');
```
instead of
```php
$value = $this->getParam('some-name');
```
as the prior implementation relied heavily on the _application_ being
the correct type.
Essentially, if you can call `$this->addParam()` in your command, you
can call `$input->getParam()`, and the workflow is completely
self-contained in your command instance.
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Since the `InputParamTrait` as originally written targets commands specifically, I have moved it to a new subnamespace in order to communicate that more clearly. In turn, I have renamed `InputParamMethodsTrait` to `InputParamTrait` within the `Input` namespace, as it helps consumers fulfill the `InputParamInterface`. Finally, I renamed `ValidatedQuestionTrait` to `StandardQuestionTrait`. The trait was not providing a validated question, but rather a standard question instance that consumers can then modify and return from their own `getQuestion()` methods. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Adopts laminas-coding-standard 2.0, and applies it. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Details the `InputParamInterface`, the traits that implement it, and the various standard implementations. Documents how to compose and access parameters in your commands. Provides information on creating custom input parameters. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Updates `TestAsset\ParamCommand` to use the `Command\InputParamTrait`, and compose an `IntParam` with the same configuration as previously used. - Updates the `InputParamTrait::run()` method to return the value of `parent::run()` (previously, returned void, which was incorrect) - Updates the `LazyLoadingCommand::execute()` method to call `run()` on the composed command, instead of `execute()`, which ensures that all parameter binding and similar has occurred correctly. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
…nstructor For consistency Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
min/max are now set via methods. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Since we want users to typehint against it. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Due to a bug in symfony/console v5 releases (symfony/symfony#37046), when specifying multiple inputs, only the first is used in test environments, which breaks tests that verify that questions re-prompt on invalid input. The solution is to set the max attempts to a reasonably high value during testing. I have accomplished that by setting an enviornment variable, which I then check for before asking the question. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
…sions symfony/console v5 adds typehints to the `InputInterface`, whereas v4 did not have them. As such, we need two variations of param aware input, targetting each version of the interface, and used only with the associated version. This patch introduces `ParamAwareInputInterface`, which extends both `InputInterface` and `StreamableInputInterface`, adding `getParam(string $name)`. It then splits `ParamAwareInput` into: - `TypeHintedParamAwareInput` (for symfony/console v5) - `NonHintedParamAwareInput` (for symfony/console v4) `Command\InputParamTrait` now varies which one it creates based on the symfony/console version detected. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
We know when we are in version 5 due to the fact that `Command\InputParamTrait` switches param aware input based on symfony/console version. As such, in the v5 variant, we just set the max attempts to a high value. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
This allows us to mark the implementations as internal, while still allowing users to typehint on the interface within their commands. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
…andTrait` Makes it more clear what the purpose is. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
In order to prevent user mistakes where they forget to set the path type when they want a directory entered, this patch modifies the `PathParam` to add the path type as a required constructor argument, and makes the `setPathType()` method private, so that it can only be set during instantiation. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
This patch converts `ParamAwareCommandTrait` into the abstract class `AbstractParamAwareCommand`, which makes it possible for end-users to override the `run()` method in their own implementations if needed. To make this possible, the patch also introduces the `final` method `decorateInputToBeParamAware()`, which accepts the input and output, and uses them to return a `ParamAwareInputInterface` implementation. This allows users overriding `run()` to do either of: ```php parent::run($input, $output) ``` or ```php $input = $this->decorateInputToBeParamAware($input, $output); ``` If you want a command to be parameter aware, it now MUST extend `AbstractParamAwareCommand`. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
I figured out the issue: it's not intelephense, it's our CS rules. It's apparently not considering the inheritance tree when determining if an argument or return value is declared with a type. I'll add rules to exclude these implementations, or at least the portions that are implementing interface names. |
- phpdoc and IDES do not need it for methods implementing documented interface methods or overriding parent methods without changes. - It _can_ be used inline when _adding_ to the docblock. - Our phpcs rules DO NOT TAKE INTO ACCOUNT INHERITANCE, so disable rules are necessary to prevent warnings within proxy classes. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
…ramInterface` Allows us to indicate that a parameter is `VALUE_REQUIRED | VALUE_IS_ARRAY`, without requiring a new parameter class. Default provided in `InputParamTrait` is `VALUE_REQUIRED`, and the `BoolParam` sets its own to `VALUE_NONE`. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
`Command::addOption()` has a signature change between v4 and v5, requiring us to have two `ParamAwareCommandStub` implementations. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
|
Okay, final changes for this PR are in place, I think:
@michalbundyra and @boesing Can I get a final review and/or merge at this time? I can then tag 0.1.0, and start working on the issue backlog. |
|
LGTM |
There was a problem hiding this comment.
@weierophinney I think it's much cleaner now with abstract class. I would do the same for InputParam - as noted in one comment - I think we should extend here the InputOption class as we duplicate a lot of the functionality of the options, and, actually, we are extending functionality of the options with our input parameters.
Disallowed WebimpressCodingStandard.Commenting.TagWithType.InvalidTypeFormat and InvalidParamName in AbstractParamAwareCommand and AbstractParamAwareInput so that we can use `array<...>` notation to describe arrays. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Marks `$params` as private. - Marks `getParam()` as final. Done to prevent userland extensions changing behavior in unexpected ways. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
We should allow only: - VALUE_NONE - VALUE_REQUIRED - VALUE_REQUIRED | VALUE_IS_ARRAY Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
812ff5e to
a1f33ef
Compare
Simpler inheritance, and allows inheriting the constructor. Primary cost is that methods in extending classes need to memoize values returned by getter methods to reduce overhead of multiple method calls. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Updates the `InputParamInterface` methods around shortcuts to reflect the values allowed by the `InputOption` constructor, which allows `null`, a string, or an array of strings. Renames the `InputParamAwareTraitTest` to `AbstractInputParamTest`, as it is the correct name now, and adds more tests. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
|
@michalbundyra Okay, I've converted InputParamTrait to AbstractInputParam, and modified the |
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
There was a problem hiding this comment.
@weierophinney thanks for the update, just several small comments. It looks better with the abstract class now! 👍
src/Input/AbstractInputParam.php
Outdated
| return; | ||
| } | ||
|
|
||
| array_walk($shortcut, function ($shortcut) { |
There was a problem hiding this comment.
callback can be static
Also adds phpspec/prophecy-phpunit, and updates tests that use prophecy for mocking to compose the `ProphecyTrait` (preventing warnings from PHPUnit). Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Instead of `array`, use `string[]`. Also, use `phpcs:ignore` instead of `phpcs:disable`, as the latter disables it until `phpcs:enable` is called, while the former only disables it for the immediately following line. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Use `===` with empty string and empty array to check for empty, instead of `empty()`. - `trim()` string shortcuts when checking for empty. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Description
As we have now command chains I am adding new feature here, another PoC.
Please see documentation in this PR for more details, and also please see updated PR in laminas-mvc: laminas/laminas-mvc#49 where this feature is consumed.
Again: not everything is perfect here, tests are missing, but this is just idea how what I want to achieve here. Any suggestions appreciated! :)