Conversation
|
@equip/contributors feedback requested. |
shadowhand
force-pushed
the
feature/version-2
branch
2 times, most recently
from
e538454 to
af3129b
Compare
| public function required() | ||
| private $options; | ||
|
|
||
| public function withOptions(LoginOptions $options) |
There was a problem hiding this comment.
Would it be beneficial to have a helper here that did the clone automatically?
public function withOptions(LoginOptions $options)
{
return $this->setOptions($options);
}and
protected function setOptions(OptionsInterface $options)
{
$copy = clone $this;
$copy->options = $options;
return $copy;
}There was a problem hiding this comment.
Is it really worth a separate trait for 3 lines of code?
There was a problem hiding this comment.
Eh, it's more like six, versus the two it would take to import and use a trait within a class. I think it's worth it.
There was a problem hiding this comment.
Is this exact logic likely to be the most common use, or would setOptions often be customized (for validation purposes or otherwise)? That strikes me as the main question here.
There was a problem hiding this comment.
Never mind, I see that options are supposed to self-validate. I vote in favor of the helper.
There was a problem hiding this comment.
Added CommandImmutableOptionsTrait.
The way that the `Command` and `Options` classes were implemented made them largely unusable in the real world. Add a new `OptionsInterface` and drop `Command` and `Options` entirely, giving preferences to custom implementation that can better reflect business requirements.
shadowhand
force-pushed
the
feature/version-2
branch
from
af3129b to
911426f
Compare
|
This is looking good! |
|
Tests are added. Anything else need to be done before I tag v2.0? |
Helps reduce duplication in command `withOptions()` methods.
|
👍 |
4 participants
The way that the
CommandandOptionsclasses were implemented madethem largely unusable in the real world. Add a new
OptionsInterfaceand drop
CommandandOptionsentirely, giving preferences to customimplementation that can better reflect business requirements.