Refactored the configuration layer, and expose a 'configure' command #43
Conversation
lyrixx
force-pushed
the
configure
branch
2 times, most recently
from
3300c2f to
018814a
Compare
| file_put_contents($configPath, json_encode($newConfig)); | ||
| } | ||
| if ($command instanceof LocalCommand\ConfigureCommand) { | ||
| return parent::doRunCommand($command, $input, $output); |
There was a problem hiding this comment.
I would rather do the opposite check to avoid going to the parent twice. Currently, your logic does not allow to have logging for the configure command (not sure if it could log something though), and it would be the same for future commands.
Btw, would it make sense to exclude the HelpCommand as well to avoid the config when displaying the command help or no ?
There was a problem hiding this comment.
Currently, your logic does not allow to have logging for the configure command (not sure if it could log something though), and it would be the same for future commands.
There is no log in configure method ;)
Btw, would it make sense to exclude the HelpCommand as well to avoid the config when displaying the command help or no ?
Yes ;)
There was a problem hiding this comment.
I fixed that in a different way. No the config-aware-able should be explicit.
lyrixx
force-pushed
the
configure
branch
2 times, most recently
from
f527b2d to
0e8f8e0
Compare
|
|
||
| public function toArray() | ||
| { | ||
| return [ |
There was a problem hiding this comment.
this does not work on 5.3 either
There was a problem hiding this comment.
btw, the fact that tests are passing on 5.3 shows that the CLI part is untested
| } | ||
|
|
||
| if (PHP_VERSION_ID > 50400) { | ||
| $question = new ConfirmationQuestion(json_encode($configuration->toArray(), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)."\n\nDo you want to save this configuration? [Y/n]"); |
There was a problem hiding this comment.
you should also unescape the unicode chars here
lyrixx
force-pushed
the
configure
branch
2 times, most recently
from
5016e89 to
3735954
Compare
|
👍 |
|
it would be worth testing the CLI layer, but it may be done separately |
|
@stof Yes, it's planned ;) |
…' command (lyrixx) This PR was merged into the 1.1.x-dev branch. Discussion ---------- Refactored the configuration layer, and expose a 'configure' command Commits ------- 4da83d4 Refactored the configuration layer, and expose a 'configure' command
No description provided.