More modernization #260

alanpoulain · 2020-09-28T10:16:12Z

Little fixes in the source code (mainly from PHP Inspections)
Use latest PHPUnit
Declare strict typehint in test files
Add typehint in test files
Replace the PHPUnit's deprecated methods

alanpoulain · 2020-09-28T10:18:22Z

bin/compile

@@ -39,7 +39,7 @@ $builder->buildFromIterator(
        ->files()
        ->name('*.php')
        ->name('*.pem*')
-        ->exclude(['Tests', 'tests', 'phpunit'])
+        ->exclude(['Tests', 'tests'])


Because the PHAR needs phpunit/phpunit/src/Framework/Assert/Functions.php, autoloaded by Composer in the latest PHPUnit.

alanpoulain · 2020-09-28T10:19:12Z

composer.json

-    },
-    "config": {
-        "platform": {
-            "php": "7.1.3"


Why locking the PHP version to 7.1.3, since all versions are tested in the CI?

#259 (comment) ?

This config is problematic because it tells Composer the PHP version installed is PHP 7.1.3 (see https://getcomposer.org/doc/06-config.md#platform).
It means that, in the CI, when PHP 7.4 is used, Composer is actually installing packages for PHP 7.1.3.

I don't know the initial reason of why the platform was set to php 7.1, I only specified the bugfix version so I could install psalm.

If there isn't a concrete reason to specify the php version it's better to remove it indeed. That being said I don't think it's problematic that composer install dependencies targeting 7.1, as long as the dependencies themselves are tested against 7.4.

I think it's problematic, because it means you cannot test Couscous against the latest dependencies.
For instance, Symfony 5 was not tested (PHP > 7.2), and, if tested, we could have seen it was failing because of the missing return codes in the commands.

alanpoulain · 2020-09-28T10:20:13Z

phpunit.xml.dist

@@ -5,7 +5,6 @@
         convertWarningsToExceptions="true"
         beStrictAboutOutputDuringTests="true"
         beStrictAboutTestsThatDoNotTestAnything="true"
-         forceCoversAnnotation="true"


Because some tests don't have the @cover annotation (and the latest PHPUnit doesn't like this).

mnapoli

I've added some comments, the PR changes a lot of things in a single commit, so it's really hard to review and be confident

mnapoli · 2020-09-29T15:27:39Z

src/Application/Cli/ClearCommand.php

@@ -29,13 +29,19 @@ public function __construct(Filesystem $filesystem)
        parent::__construct();
    }

+    /**
+     * {@inheritdoc}
+     */


Could you please revert these? (it's adding unnecessary noise)

Sure. Removing it everywhere instead then.

mnapoli · 2020-09-29T15:28:03Z

src/Application/Cli/PreviewCommand.php

@@ -237,7 +237,7 @@ private function isFound(string $executablePath): bool

    private function fileListToDisplay(array $files, string $sourceDirectory): string
    {
-        $files = array_map(function (string $file) use ($sourceDirectory): string {
+        $files = array_map(static function (string $file) use ($sourceDirectory): string {


Same for these, they are not useful and add noise.

It's a suggestion from PHP Inspections EA:

[EA] This closure can be declared as static (better scoping; in some cases can improve performance).

Please tell me if you really want to remove them, I will do it.

mnapoli · 2020-09-29T15:28:25Z

src/Application/Cli/TravisAutoDeployCommand.php

@@ -93,23 +93,23 @@ protected function execute(InputInterface $input, OutputInterface $output): int
        if ($travisBranch !== 'master') {
            $output->writeln('<comment>[NOT DEPLOYED] Deploying Couscous only for master branch</comment>');

-            return 0;
+            return 1;


This is a behavior change, not sure we want that. Why change that? (same for the lines below)

Is it really a behavior change?
The return values have been added in the #259 PR.
Also it means the command has failed (EXIT_FAILURE), isn't it better? (can be useful when chaining commands in Bash for instance).

When I added them I used 0 as the output only contains comments and no errors (and because that's the default value set by Symfony).

As far as I understand how the CI is configured this exit code won't change the behaviour of the CI as the command is run on after_success

IMO, the comments look like errors, and that's why I changed the return codes, but I could be wrong.

mnapoli · 2020-09-29T15:28:52Z

src/CommandRunner/Git.php

@@ -61,11 +61,13 @@ public function getRemoteUrl(string $remote = 'origin'): string
     * Check if the git repository in the provided directory has any uncommitted changes
     *
     * @param string $directory A directory containing a git repository
+     *


I don't see a reason to do this

It is a CS fix to be coherent with the rest of the codebase (there is a new line before every @return everywhere else).

mnapoli · 2020-09-29T15:28:57Z

src/CommandRunner/Git.php

     * @return bool True if there are changes, false otherwise
     */
    public function hasUncommittedChanges(string $directory): bool
    {
        $changes = $this->run($directory, "git diff-index --name-only HEAD");
+


It is also a CS fix to be coherent with the rest of the codebase.
It's a common CS (for instance in Symfony).

mnapoli · 2020-09-29T15:32:20Z

composer.json

-    },
-    "config": {
-        "platform": {
-            "php": "7.1.3"


#259 (comment) ?

alanpoulain · 2020-09-29T15:55:09Z

I've added some comments, the PR changes a lot of things in a single commit, so it's really hard to review and be confident

Oh sorry for that. I thought it was OK since it's mainly typehint in test files (and the tests are green).
If you really prefer I can try to separate it in different commits (src files, typehint for test files, replacement of deprecated methods).
Please tell me if you need this.

Baptouuuu · 2020-09-29T18:27:33Z

src/Module/Template/Step/ValidateTemplateDirectory.php

@@ -14,7 +14,7 @@
 */
 class ValidateTemplateDirectory implements Step
 {
-    const DEFAULT_TEMPLATE_DIRECTORY = 'website';
+    public const DEFAULT_TEMPLATE_DIRECTORY = 'website';


Why make this const public when the others are private ?

It's because this const is also used here:

Couscous/src/Module/Template/Step/UseDefaultTemplate.php

Line 53 in c168d0b

$templateDirectory = $project->sourceDirectory.'/'.ValidateTemplateDirectory::DEFAULT_TEMPLATE_DIRECTORY;

alanpoulain · 2020-10-11T16:12:23Z

Do you want me to revamp this PR? Maybe by doing small PRs instead?

mnapoli · 2020-11-14T18:20:23Z

Do you want me to revamp this PR? Maybe by doing small PRs instead?

Hi, yes 😅 I'm sorry, reviewing PRs takes a lot of time across all the repositories, and this one is huge and not top priority so I have a lot of trouble saving time for this.

Let's move forward and close this one, I don't think I'll be able to merge it. If you still want to, smaller (targeted) PRs would work better I think.

Thanks for your efforts and patience!

More modernization

637f567

alanpoulain commented Sep 28, 2020

View reviewed changes

mnapoli reviewed Sep 29, 2020

View reviewed changes

chore: remove {@inheritdoc} from commands methods

8988782

Baptouuuu reviewed Sep 29, 2020

View reviewed changes

mnapoli closed this Nov 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More modernization #260

More modernization #260

alanpoulain commented Sep 28, 2020

alanpoulain Sep 28, 2020

alanpoulain Sep 28, 2020

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

Baptouuuu Sep 29, 2020

alanpoulain Sep 30, 2020

alanpoulain Sep 28, 2020

mnapoli left a comment

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

Baptouuuu Sep 29, 2020

alanpoulain Sep 30, 2020

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

mnapoli Sep 29, 2020

alanpoulain Sep 29, 2020

mnapoli Sep 29, 2020

alanpoulain commented Sep 29, 2020

Baptouuuu Sep 29, 2020

alanpoulain Sep 30, 2020

alanpoulain commented Oct 11, 2020

mnapoli commented Nov 14, 2020

More modernization #260

More modernization #260

Conversation

alanpoulain commented Sep 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnapoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain commented Sep 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain commented Oct 11, 2020

mnapoli commented Nov 14, 2020