AL-778 - Added update notifications #1523
Conversation
|
@TeslaDethray, thanks for your PR! By analyzing the blame information on this pull request, we identified @greg-1-anderson, @ronan and @drainpip to be potential reviewers |
TeslaDethray
changed the title
TeslaDethray
force-pushed
the
add/update_notifications
branch
from
73ac344
to
2649d5e
Compare
TeslaDethray
force-pushed
the
add/update_notifications
branch
6 times, most recently
from
3585316
to
f3573c9
Compare
TeslaDethray
force-pushed
the
add/update_notifications
branch
from
f3573c9
to
28b823d
Compare
| @@ -24,7 +27,7 @@ class TerminusException extends \Exception | |||
| */ | |||
| public function __construct( | |||
| $message = null, | |||
| $replacements = array(), | |||
| array $replacements = [], | |||
There was a problem hiding this comment.
I'm curious why you're adding a type hint here when we're not doing that pretty much anywhere else? I'm not against using them, but we should be clear on what the accepted rule is for them.
There was a problem hiding this comment.
Yes, we do use them.
grep -r ', array' TERMINUS_ROOT/src/* | grep -v _keys
| @@ -138,23 +139,35 @@ public function run(InputInterface $input, OutputInterface $output) | |||
| $status_code = $this->runner->run($input, $output, null, $this->commands); | |||
| if (!empty($cassette) && !empty($mode)) { | |||
| $this->stopVCR(); | |||
| } else { | |||
| $this->runUpdateChecker(); | |||
There was a problem hiding this comment.
I still don't love that this outputs after the command instead of before. I understand the desire to have it not be ignored but this feels aggressive. All of the other cli's I have experience with put this warning first.
There was a problem hiding this comment.
Also, I wonder if this should happen on startup rather than on run. If we do implement a shell command that would be more appropriate. Same if we have commands that call the runner to run other commands.
There was a problem hiding this comment.
Running at the end is what Product wants.
|
👍 On technical implementation (with the one change regarding not pulling the container back out of the runner when needed). I'd still argue against the user experience of putting the update after the command output (since it's non-standard). And would put the check in the terminus init stage (so that a future shell mode doesn't become a hot mess). But those are product decisions and this meets the ACs. |
…to Pantheon\Terminus\Terminus
TeslaDethray
force-pushed
the
add/update_notifications
branch
from
bfcc48d
to
6c0e24e
Compare
Closes #1497
Closes #1304