Apply coding style

[lehecht]

.php_cs is outdated and cannot be used anymore due to PHP CS Fixer 3 upgrade. Instead use .php-cs-fixer.php and updated methods.

[mzur]

Thanks! Please also update the fix script in composer.json to use the new config file.

Beside the comments below, a problem I always had is the indentation that CS fixer wants to enforce. For example, it suggests something like this:

         $this->putJson("/api/v1/maia/annotation-candidates/{$a->id}", [
-                // Must be points of a circle.
-                'points' => [10, 20],
-            ])
+            // Must be points of a circle.
+            'points' => [10, 20],
+        ])
             ->assertStatus(422);

But I want the parenthesis of putJson() to end at the same level of indentation than the chained assertStatus(). Either the parenthesis (and arguments) are indented one level or the chained method is unindented. Can you find a ruleset that does this? You can check the proposed output with composer fix -- --dry-run --diff.

[mzur]

Also please remember to put the Resolves #651 in the commit message body. This makes the commit title more readable and GitHub will automatically link the issue to the PR 😉 Example:

Fix name of .php_cs file

.php_cs is outdated and cannot be used anymore
due to PHP CS Fixer 3 upgrade. Instead use
.php-cs-fixer.php and updated methods.

Resolves #651

[lehecht]

Ah yes. Sorry, I completely forgot about this. Shall I change this and force push the changes?

[mzur]

Force push because of the commit message? No, it's fine. Just do it next time 😉

[mzur]

I've opened a discussion for the method chain indentation issue: PHP-CS-Fixer/PHP-CS-Fixer#7319

[mzur]

In the discussion, the use_arrow_functions rule was suggested. I think I'd like to include this rule here, too.

[mzur]

So, one possible solution (from the discussion linked above) would be to make sure the line with the opening [ or { is properly indented like this:

$this
    ->putJson("/api/v1/maia/annotation-candidates/{$a->id}", [
        // Must be points of a circle.
        'points' => [10, 20],
    ])
    ->assertStatus(422);

or this:

$parentConflicts = 
    $mergeLabels->filter(function ($label) {
        return array_key_exists('conflicting_parent_id', $label);
    })
    ->each(function ($label) use ($parentConflictResolution) {

This should work both for the multiline array case and for the multiline closure case. I could live with this coding style but unfortunately all these cases would have to be updated by hand. We could do this all with great effort now and then enforce the style in the future. On the other hand, this is a cosmetic change and might cost a lot of time for "nothing". @lehecht Could you estimate how much manual work would be involved?

[lehecht]

Should something like

 $query = $this->volume->files()
  ->when($this->only, function ($query) {
   return $query->whereIn('id', $this->only);
  });

be changed to this

 $query = $this
  ->volume->files()
  ->when($this->only, fn ($query) => $query->whereIn('id', $this->only));

or to this ?

 $query =
 $this->volume->files()
  ->when($this->only, fn ($query) => $query->whereIn('id', $this->only));

[lehecht]

Could you estimate how much manual work would be involved?

I wrote a small python parser to help recognize some lines with wrong indentation and fix them. This helps a lot but will still require proof reading. Furthermore testing is needed. I wouldn't rely on the composer test command alone since some bugs could occur after using several functions consecutively. So I guess it will take between 3-4 days.

[mzur]

I would change your example from

$query = $this->volume->files()
   ->when($this->only, function ($query) {
      return $query->whereIn('id', $this->only);
   });

to

$query =  $this->volume
   ->files()
   ->when($this->only, fn ($query) => $query->whereIn('id', $this->only));

To be more consistent, I'll update my example from above to

$parentConflicts = $mergeLabels
    ->filter(function ($label) {
        return array_key_exists('conflicting_parent_id', $label);
    })
    ->each(function ($label) use ($parentConflictResolution) {

It's also only a problem with the CS Fixer if the closure or array is opened at a different level of indentation than it should be closed (see my examples above). In your example 1) the closure starts at the same level than it should be closed and 2) in the updated code there is no multi-line closure at all any more.

I wouldn't rely on the composer test command alone since some bugs could occur after using several functions consecutively.

If that's the case then we sould update the tests 😉 Do you have an example for that?

So I guess it will take between 3-4 days.

That's a lot but I think it's worth it. Afterwards we can enforce the coding style in pull requests.

[lehecht]

I applied your changes and a new rule (no_unused_imports) to the app directory. Could you look through some files to check if you like it? I will apply those changes to all other files after you gave me your feedback.

Some candidates to check could be:

• app/Jobs/CloneImagesOrVideos.php
• app/AnnotationSession.php

[lehecht]

If that's the case then we sould update the tests 😉 Do you have an example for that?

No, I don't have one. I just meant that more complex bugs sometimes occur when several functions were used consecutively. But maybe I'm worrying too much about it. I also don't know how every test looks like. So if you think, it's enough to just run composer test then I will just do that.

[mzur]

There are many changes that I wouldn't do. I've tried to give a few examples.

If this is getting too complicated, let me know. We could always disable any indentation style checks, too.

[mzur]

I saw that you marked this ready for review and I'll have a look. Next time please also click to re-request my review otherwise I won't get a notification for this 😉

[mzur]

Looks like a nice cleanup! I really like all the arrow functions that we now got for free and that make the code more readable.

I've added several suggestions that should make the code even more readable in context of what we agreed on here. It's probably easiest for you if you commit them as a batch here in the GitHub UI (if you agree with them).

Do you also want to add the GitHub action that will complain about incorrect coding style in this PR?

When this is all sorted out and working, the modules come next 😉 But maybe you should work on something else in the meantime so you don't do CS fixed for the rest of the year 😄 You could pick up one module after the other inbetween other tasks (bugfixes, features).

[lehecht]

Looks like a nice cleanup! I really like all the arrow functions that we now got for free and that make the code more readable.

I've added several suggestions that should make the code even more readable in context of what we agreed on here. It's probably easiest for you if you commit them as a batch here in the GitHub UI (if you agree with them).

Thank you for your big support! I'm sorry that you still had to put so much time into it. I really just went through all marked files of git diff.

[lehecht]

Do you also want to add the GitHub action that will complain about incorrect coding style in this PR?

Yes! But I will need some more informations on how to do it.

[mzur]

There are 3 unresolved comments from my previous review.

I think we are almost done now. But I suggest that the GitHub action should be added in another PR so we can merge this huge one first.

When you want to implement a new GitHub action, you add a new file to .github/workflows. However, we already have lint.yml there to which the new job could be added, too. As a start, all you have to do is to copy the lint-php block and append it as new cs-php block to the file. Then you only have to update the last command from composer lint to composer fix -- --dry-run. After that we can try to figure out how to add GitHub annotations.

[mzur]

This has to be updated, too.

[lehecht]

After that we can try to figure out how to add GitHub annotations.

Unfortunately, the given website states that PHP-CS Fixer doesn't support GitHub annotations.

[mzur]

The linked article suggests a solution with php-cs-fixer fix -vvv --dry-run --format=checkstyle | cs2pr. We can try that in #677.