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]

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 :wink: 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 :wink:

[mzur]

I've opened a discussion for the method chain indentation issue: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/discussions/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]

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 :wink:

[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.

[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.