[FrameworkBundle] Refactored assets:install command and apply Symfony styles #14235
Conversation
|
👍 IMO this is way better than your previous proposal. But there is still an issue (raised by @stof : #13057 (comment) ) using the table layout, which doesn't allow to stream the output. However, I don't think streaming the output for this command is an absolute requirement. |
|
I was thinking about using a list too, but streaming the output with list is not possible for now either (at least using
Exceptions are caught and printed to the user. |
|
As we're currently loosing the paths information, do you think the verbosity level should allow to print it in the bundle column ? BTW, about the list and SymfonyStyle: IMO we do not have to use the SymfonyStyle exclusively, but to respect as much as possible the standards and the console style guide. That doesn't mean we cannot use our own way to print particular informations. Perhaps it will create new standards for use-cases not covered by the current guide. |
|
I'm not sure of using UTF-8 characters as they appearance in the console depends on a lot of things. |
ef58b13 to
df14ee3
Compare
| @@ -11,20 +11,35 @@ | |||
|
|
|||
| namespace Symfony\Bundle\FrameworkBundle\Command; | |||
|
|
|||
| use Exception; | |||
There was a problem hiding this comment.
This one should be removed and a \ added when needed in the code.
|
How about a progress bar to indicate running? You can show the table after its done (as summary) but having no output may cause users to think its stuck or something. |
|
This command should be really fast to execute, so I don't think fixing the "not streaming" problem is worth it. |
|
ping @fabpot |
|
@fabpot are there anything else I should do with this? |
|
👍 for merge into 2.8. |
|
@javiereguiluz what do you think about this? |
| } | ||
| $this->filesystem->symlink($originDir, $targetDir); | ||
| if (!file_exists($targetDir)) { | ||
| throw new IOException(sprintf('Symbolic link "%s" is created but appears to be broken.', $targetDir), 0, null, $targetDir); |
There was a problem hiding this comment.
... is created but ... -> ... was created but ... ?
|
👍 thanks @1ed. Very nice work! Just solve the fabbot issue and we're good to merge :) |
aaf764e to
ea3b3a7
Compare
|
Ready. Thank you! |
|
Thank you @1ed. |
Replaces #13057