Generic.Files.LineLength: Add ignoring of phpcs:ignore

[o5]

Goal

It would be nice to not count // phpcs:ignore Some.Sniff to total length of line because of writing something like // phpcs:ignore Some.Sniff, Generic.Files.LineLength is a little bit useless I think.

Use case

Class Foo
{
    ...

    private function configurePreflightHandler(Application $application): void
    {
        /** @var RouteInterface $route */
        $route = $application->options(self::ROUTE_PATTERN, function (Request $request, Response $response) { // phpcs:ignore SlevomatCodingStandard.Functions.StaticClosure.ClosureNotStatic
            return $response;
        });

        $route->setAttribute(Request::ROUTE_GENERIC, true);
    }
}

Rule settings:

<rule ref="Generic.Files.LineLength">
    <properties>
        <property name="lineLimit" value="130"/>
        <property name="absoluteLineLimit" value="0"/>
        <property name="ignoreComments" value="true"/>
    </properties>
</rule>

Current state

FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------
 73 | WARNING | Line exceeds 130 characters; contains 189 characters
    |         | (Generic.Files.LineLength.TooLong)
 ...
-----------------------------------------------------------------------------------

State with this patch

No warnings found.

Additional info

I prepared this PR without new configuration but if you need it, just let me know and I'll add something similar like $ignoreComments :)

[o5]

No idea why one job in Travis failed :/

[o5]

Travis has been fixed.

[o5]

@gsherwood Is there something wrong? Can I help?

[gsherwood]

@o5 I haven't had a chance to review this yet - it's still sitting in my gmail inbox - but it sounds like a very good change to me. I'll add it to the 3.5.0 milestone for the next release, but I'm not 100% sure when I'll get it merged.

[gsherwood]

I've had a look at this now and think there is actually a problem with the sniff itself. The $ignoreComments setting is supposed to ignore lines that only contain comments. What it is actually doing is ignoring all lines that end with a comment, no matter how short that comment is.

The way phpcs:ignore works is that it will ignore the line of code it is on, or it will ignore the next line of code if it is on a line by itself. So you'd have to rewrite your code like this (assuming the @var annotation still works):

Class Foo
{
    ...

    private function configurePreflightHandler(Application $application): void
    {
        /** @var RouteInterface $route */
        // phpcs:ignore SlevomatCodingStandard.Functions.StaticClosure.ClosureNotStatic
        $route = $application->options(self::ROUTE_PATTERN, function (Request $request, Response $response) {
            return $response;
        });

        $route->setAttribute(Request::ROUTE_GENERIC, true);
    }
}

The LineLength sniff will already ignore lines that contain only PHPCS annotations, so no change is required to get this working.

What you may actually be after is a feature of the sniff to not count the length of trailing comments in the line length calculation, but that doesn't currently exist.

I'm going to need to change the ignoreComments feature to get it working as it was originally intended. If a ignoreTrailingComments feature is of interest to you, I could have a look at that too.

[gsherwood]

I've had another thought. The sniff currently ignores lines that end with comments if the comment itself cannot be broken up onto multiple lines (e.g., when it contains a long URL that you cant split up). Given PHPCS annotation cannot be split up, maybe they should count as unsplitable comments at the end of lines, and also be ignored. Not just the phpcs:ignore annotation, but all of them.

I think I still need to change the sniff to make sure a line > max length still throws an error even if it ends with a comment, but I don't think that's controversial.

[o5]

The way phpcs:ignore works is that it will ignore the line of code it is on, or it will ignore the next line of code if it is on a line by itself. So you'd have to rewrite your code like this (assuming the @var annotation still works):

Thanks for this tip! It makes sense to me for use-case which I provided, but I checked our code-base and discovered an another use-case.

/** @var RouteInterface $route */
$route = $application->post(
    self::TEST_URI,
    function (Request $request, Response $response) use (&$called): Response { // phpcs:ignore SlevomatCodingStandard.Functions.StaticClosure.ClosureNotStatic
        TestCase::assertInstanceOf(Payload::class, $request->getJwtPayload());
        TestCase::assertNotNull($request->getJwtPayload()->getClientId());
        TestCase::assertSame('client', $request->getJwtPayload()->getClientId()->toString());

        $called = true;

        return $response;
    },
);

Unfortunately, your "workaround" in this case looks a little bit confusing for me (have comment between function parameters). But sure, it can be confusing only for me and in this time. I'll think about it :)

Feature ignoreTrailingComments sounds better for me :)

[gsherwood]

I've had yet another thought. I think ignoreComments could reasonably be assumed to mean that comments are ignored when calculating line length. So any comments at the end of a line could be ignored and not counted in the overall line length. If a line is only a comment, removing the comment from the calculation has the same effect as ignoring the line completely, so it probably works there as well, although bailing out early is probably still better.

This change would negate the need for an ignoreTrailingComments property as that is effectively what ignoreComments would now be doing.

[gsherwood]

I think ignoreComments could reasonably be assumed to mean that comments are ignored when calculating line length.

I've now made this change and your sample code passes with no errors or warnings.

I also changed the code that checks if a comment can be wrapped so that it only applies to lines that only contain comments. Not a concern for this issue specifically, but does mean that the sniff is now back to working the way it was originally intended to when it was first written.

I'm going to close this now as the change will be in 3.5.0 and allow your code to pass without you needing to do anything, but I can't merge the PR as the sniff is now quite different in order to achieve this.

Thanks for bringing this issue to my attention - the sniff is much better for it.

[o5]

@gsherwood I've tried the master (204dcd6) version of squizlabs/php_codesniffer in our project (LOC 50000) and I can confirm it works well :)

Thank you!

[gsherwood]

@o5 Great news, thanks a lot for testing it.