-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[AssetMapper] Adding 'Everything up to date' message #59178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AssetMapper] Adding 'Everything up to date' message #59178
Conversation
src/Symfony/Component/AssetMapper/Command/ImportMapOutdatedCommand.php
Outdated
Show resolved
Hide resolved
@@ -76,6 +76,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
$packagesUpdateInfos = $this->updateChecker->getAvailableUpdates($packages); | |||
$packagesUpdateInfos = array_filter($packagesUpdateInfos, fn ($packageUpdateInfo) => $packageUpdateInfo->hasUpdate()); | |||
if (0 === \count($packagesUpdateInfos)) { | |||
$io->writeln('Everything is up to date'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should respect the format option here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. But what would be the JSON version? Something like:
{"message": "Everything is up to date"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be considered as a reporting message rather than an output message.
For JSON, the only sensible output would be an empty array (returning a different JSON schema would break any scripting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, returning []
now :-)
If packages are passed to the command, what about using "Package XX and YY up to date" ... or find something similar. As "everything is up to date" would be missleading. |
Globally about this PR composer uses "everything" but just after precising what and this feels more contextually correct So here i'd either
wdyt ? composerNothing outdatedcomposer outdated
Direct dependencies required in composer.json:
Everything up to date
Transitive dependencies not required in composer.json:
Everything up to date Something outdatedcomposer outdated
Color legend:
- patch or minor release available - update recommended
- major release available - update possible
Direct dependencies required in composer.json:
doctrine/dbal 3.8.3 4.2.1 Powerful PHP database abstraction layer (DBAL) with many features for database schem...
doctrine/doctrine-bundle 2.12.0 2.13.1 Sy
(...) npmNothing outdated$ npm outdated
Something outdated$ npm outdated
Package Current Wanted Latest Location Depended by
date-fns 3.6.0 3.6.0 4.1.0 node_modules/date-fns simonandre
lightningcss-cli 1.27.0 1.28.2 1.28.2 node_modules/lightningcss-cli simonandre
(...) brew: no outputNothing outdated$ brew outdated
Sometimes echo some logs about cask files downloaded... but no message returned Something outdated$ brew outdated
You have 70 outdated formulae and 2 outdated casks installed.
act (0.2.68) < 0.2.70
awscli (2.18.11) < 2.22.17
blackfireio/blackfire/blackfire (2.28.18) < 2.28.21
buf (1.45.0) < 1.47.2
(...) or $ brew outdated
brew outdated
blackfireio/blackfire/blackfire (2.28.18) < 2.28.21
git (2.47.0) < 2.47.1
git-lfs (3.5.1) < 3.6.0
gitlab-runner (17.5.2) < 17.6.0
libgit2 (1.8.2) < 1.8.4
yarn: no output (?)Yarn does not seem to return anything when nothing is outdated.... but not sure the version i have is the most recent 😆 |
IMO, a command should always give some output - to indicate that it did run.
? |
Yep this would work I guess! Maybe a small test to check json does not output something ? |
Sorry, I'm stuck with the tests :-( Is ImportMapVersionCheckerTest the right place? But don't understand what's going on in there... |
Hop: 🎁 <?php
/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace Symfony\Component\AssetMapper\Tests\ImportMap;
use PHPUnit\Framework\TestCase;
use Symfony\Component\AssetMapper\Command\ImportMapOutdatedCommand;
use Symfony\Component\AssetMapper\ImportMap\ImportMapUpdateChecker;
use Symfony\Component\Console\Tester\CommandTester;
class ImportMapOutdatedCommandTest extends TestCase
{
/**
* @dataProvider provideNoOutdatedPackageCases
*/
public function testCommandWhenNoOutdatedPackages(string $display, ?string $format = null): void
{
$updateChecker = $this->createMock(ImportMapUpdateChecker::class);
$command = new ImportMapOutdatedCommand($updateChecker);
$commandTester = new CommandTester($command);
$commandTester->execute(is_string($format) ? ['--format' => $format] : []);
$commandTester->assertCommandIsSuccessful();
$this->assertEquals($display, trim($commandTester->getDisplay(true)));
}
/**
* @return iterable<array{string, string|null}>
*/
public static function provideNoOutdatedPackageCases(): iterable
{
yield 'default' => ['[OK] No outdated packages found.', null];
yield 'txt' => ['[OK] No outdated packages found.', 'txt'];
yield 'json' => ['', 'json'];
}
} This is almost all you need for this one i suppose (not your fault if there were no tests before 😆 ) |
Thanks!
|
Yep perfect! Test seem to pass no? Locally you should update your dependencies I suppose :) AssetMapper & ImportMap commands are only available for Symfony 6.3+ (or 6.4 ?) |
I fixed the coding style. I guess the failure of this 1 test suite is unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Could you update the changelog as well?
This is getting released as feature in 7.3, right? But where is the changelog for 7.3? |
Yes indeed, so you'll have to add it here: https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/AssetMapper/CHANGELOG.md?plain=1#L4 |
We only document new features and BC breaks in the changelog files. So to me there's nothing to document here. |
Oh ok, my bad! Everything's fine then |
5666a0d
to
2bfcd89
Compare
Thank you @ThomasLandauer. |
Adding friendly message for
php bin/console importmap:outdated
if no updates are found.Wording is taken from
composer outdated