PhpdocToCommentFixer - Add ignored_tags option

[VincentLanglet]

Sometimes, the docblock are necessary inside the code. This is the case for

/** @psalm-suppress */

This option allow to ignore some comments.

Closes: #5505

[coveralls]

Coverage increased (+0.008%) to 92.189% when pulling db400cf on VincentLanglet:feature/ignoreTag into e5ebc7f on FriendsOfPHP:master.

[VincentLanglet]

Hi @keradus, this is my first PR on this project.
I'm sorry but I don't understand the test failure.

[kubawerlos]

@VincentLanglet to fix failing tests you have to regenerate docs: php ./dev-tools/doc.php.

[VincentLanglet]

@VincentLanglet to fix failing tests you have to regenerate docs: php ./dev-tools/doc.php.

But this command is changing the example 2.
It's like it's not using my second code sample.

[kubawerlos]

But this command is changing the example 2.
It's like it's not using my second code sample.

I don't thin I get it - you already updated the docs with example 2 (as it's visible in diff), but it looks like you made some changes later. I believe if you regenerate docs the diff in this PR will look correctly.

[VincentLanglet]

I don't thin I get it - you already updated the docs with example 2 (as it's visible in diff), but it looks like you made some changes later. I believe if you regenerate docs the diff in this PR will look correctly.

The doc update does

    --- Original
    +++ New
-   @@ -1,7 +1,7 @@
+   @@ -1,12 +1,12 @@
     <?php
     $first = true;// needed because by default first docblock is never fixed.
 
-   -/** This should be a comment */
-   +/* This should be a comment */
+   -/** This will be changed */
+   +/* This will be changed */
+    foreach($connections as $key => $sqlite) {
+        $sqlite->open($path);
+    }
+
+    /** @todo This will not be changed */
     foreach($connections as $key => $sqlite) {
         $sqlite->open($path);
     }

Basically it replaces my second example by the first one.

[kubawerlos]

Basically it replaces my second example by the first one.

@VincentLanglet not really, these are the changes from - your - 2nd example, they are just produce exactly the same as 1st example. To see it more clearly change something in your example, like variable $connections to $array and regenerate the docs.

[VincentLanglet]

Basically it replaces my second example by the first one.

@VincentLanglet not really, these are the changes from - your - 2nd example, they are just produce exactly the same as 1st example. To see it more clearly change something in your example, like variable $connections to $array and regenerate the docs.

Oh, I see.

Since

/** @todo This should be a PHPDoc as the tag is on "ignored_tags" list */
foreach($connections as $key => $sqlite) {
    $sqlite->open($path);
}

is untouched, it's not in the diff !

[kubawerlos]

Exactly, I address this issue in #5585

[kubawerlos]

Let's have 2 more test cases:

• one with tag in different casing in code and config
• with PHPDoc having 2 different tags and only one of them in config

[VincentLanglet]

Done @kubawerlos :)

[kubawerlos]

@VincentLanglet for "PHPDoc having 2 different tags and only one of them in config" I meant PHPDoc like this:

/**
 * @foo text
 * @bar text
 */

and ignored tags only foo.

[VincentLanglet]

@kubawerlos Done, tahnks to this test, I discovered I should use matchAll. :)

[kubawerlos]

@VincentLanglet I cannot fint that test with PHPDoc having multiple tags

[VincentLanglet]

@VincentLanglet I cannot fint that test with PHPDoc having multiple tags

I have added an example but no tests.
But now there is also a test with multiple tags.

[kubawerlos]

The last question for this PR is: should target aim master or 3.0 instead of 2.18?

@keradus how should new features be handled now? There is not milestone 2.19 - does it mean 2.18 is last minor in v2 series and new features should aim 3.0?

[keradus]

(each new) feature should be targetting master, imho
under which tag it would get released is another story, not blocking here

[VincentLanglet]

@keradus @kubawerlos Is there any interest in this PR or should I close it ?

[julienfalque]

@VincentLanglet I think this feature is interesting and should be merged, but this requires time and maitainers don't have much these days, please be patient :)

[VincentLanglet]

@VincentLanglet I think this feature is interesting and should be merged, but this requires time and maitainers don't have much these days, please be patient :)

Sure, I understand this. I was just expecting at least one comment to be sure I don't rebase (and fix the conflict) regularly for nothing.

The branch is now up to date. Let's wait and see 🤞🏻 =)

[kubawerlos]

This is definitely something worth merging, but only @keradus, @SpacePossum and @julienfalque can do it, approving running workflow before that.

[VincentLanglet]

Friendly ping @keradus

[VincentLanglet]

@keradus Would it be possible to get some review ? :)

[SpacePossum]

I think we could do with 2 examples, one with default config and one with a single excluded tag.
Maybe we should document that the excluded tags are matched case insensitive as well.

[VincentLanglet]

I think we could do with 2 examples, one with default config and one with a single excluded tag.
Maybe we should document that the excluded tags are matched case insensitive as well.

There is already 4 examples

• One with default config
• One with single excluded tag
• One to show it's cade insensitive
• One to show it support multiple tag in the same docblock

[SpacePossum]

Yeah I saw the 4, but I think 2 is enough if we would update the docs stating the ignored tags are matched case insensitive.

Having more tests would be better than more examples. Like a test with @ignore-me for the special chars from the regex, @TODO to test the case insensitive matching works, @TODOnot to test the word matching works (so it doesn't match when configured 'todo'). What would be a sane tag look like to have the \\\\ in the regex?

[VincentLanglet]

Done @SpacePossum

[SpacePossum]

thanks 👍 if the pipeline succeeds than RTM from me

[VincentLanglet]

thanks 👍 if the pipeline succeeds than RTM from me

Tests were failing because of the doc, it might be better now I hope.

[keradus]

Thank you @VincentLanglet.