Report Full: fix delimiter line bug

[jrfnl]

Description

When determining the max message length, the calculation did not take potential explicit multi-line messages into account and would base the delimiter line length on the length of the complete message, not on the length of the individual lines.

Fixed now.

Suggested changelog entry

• Report Full: fixed bug in delimiter line calculation

Related issues/external references

Discovered while reviewing #125
Related to squizlabs/PHP_CodeSniffer#2093

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Additional information

Testing this fix manually is not that easy and there are no unit/integration tests for the reports yet either.

How I tested this myself is by taking the test code from squizlabs/PHP_CodeSniffer#2093 and adding it (temporarily) to a random sniff:

$message = "Lorem ipsum dolor sit amet, consectetur adipiscing elit.\nAenean felis urna, dictum vitae lobortis vitae, maximus nec enim. Etiam euismod placerat efficitur. Nulla eu felis ipsum.\nCras vitae ultrices turpis. Ut consectetur ligula in justo tincidunt mattis.\n\nAliquam fermentum magna id venenatis placerat. Curabitur lobortis nulla sit amet consequat fermentum. Aenean malesuada tristique aliquam. Donec eget placerat nisl.\n\nMorbi mollis, risus vel venenatis accumsan, urna dolor faucibus risus, ut congue purus augue vel ipsum.\nCurabitur nec dolor est. Suspendisse nec quam non ligula aliquam tempus. Donec laoreet maximus leo, in eleifend odio interdum vitae.";

$phpcsFile->addWarning($message, 0, 'Test');

and then running it over the simple test file from #124:

<?php

$x=1;

using the following command:

phpcs -s ./phpcs-3924.php --report-width=100000 --basepath=. --standard=universal

(the universal is because the random sniff I added the code to was in that standard)

Result when running this on master:

Result when running this against the PR branch:

[anomiex]

Looks good to me. One suggestion inline.

[anomiex]

It seems like lines 67–70 just above could go in an else block to this if, since when this if is true we ignore the results of that code.

Or we could just always do the block being added here. It'll do the right thing even if there are no newlines, it'll just do a bit more work to do it. Depends how much we value speed over code complexity here, most messages shouldn't need the extra work.

[jrfnl]

@anomiex Thanks for taking a look!

It seems like lines 67–70 just above could go in an else block to this if, since when this if is true we ignore the results of that code.

The else would still need to be an else if with the ($showSources === true) condition, so I don't see much advantage in that as it raises the cognitive load.

Or we could just always do the block being added here. It'll do the right thing even if there are no newlines, it'll just do a bit more work to do it. Depends how much we value speed over code complexity here, most messages shouldn't need the extra work.

As messages with new lines in them are very much the exception, not the rule, I'd favour performance in this case.

On that note, I do see another little tweak which can be made to remove a duplicate function call.

[anomiex]

It wouldn't just be an else if, it would be like

if (strpos($error['message'], "\n") === false) {
    // Single line, calculate length.
    $length    = strlen($error['message']);

    if ($showSources === true) {
        $srcLength = (strlen($error['source']) + 3);
        $length += $srcLength;
    }
} else {
    // Multi-line, find length of longest line.
    $errorLines = explode("\n", $error['message']);
    $length     = max(array_map('strlen', $errorLines));

    if ($showSources === true) {
        $lastLine = array_pop($errorLines);
        $srcLength = (strlen($error['source']) + 3);
        $length   = max($length, (strlen($lastLine) + $srcLength));
    }
}

(I switched around the condition there because I find it reads better with the common one-line case first)

[anomiex]

To be clear, even if I did have any veto power here to demand changes (which I don't), I wouldn't use it on this anyway if you still feel strongly about it. I'd just approve and go.

[jrfnl]

Well, for me it would be the other way round, if the code had been submitted like that (well, without the unnecessary duplicate $srcLength definition), I'd approve & merge it. 😁

When writing code myself though, I am very mindful or both performance and cognitive load/nesting levels in code.
The "how much time would it take me to grok this code if I have to look at it again in two years time ?" question.

As the single line code only contain 1 function call and 1 simple calculation which is specific to the single line code (the second function call result is used by both), the performance impact of the way the code is written now is negligible and the nesting levels and the cognitive load is lower (at least for me) 😉.

[anomiex]

I think we both think either version of the code is good enough and we'd both defer to the author's preference in this situation. 🙂

Personally I don't mind the separate setting of $srcLength in both code paths, since only one will run and moving it out without potentially setting it unnecessarily would mean having to duplicate the if ($showSources === true) { check. Then again, if I were writing it entirely myself I'd probably not have $srcLength at all as I'd find inlining the strlen call into the expressions easier to follow.

Getting further off topic: I've always found judging cognitive load tricky, I've had plenty of experience both with code I find easy to understand that others tell me they find hard and code I find hard to follow that others tell me they find easier. I've never been able to get a grasp on to what extent that's because I personally think differently from most people and to what extent it's that everyone thinks differently from others.

[jrfnl]

I hear you. Let's just say that for now, this patch has had enough love & attention and the code can go in. A future iteration (maybe when I work on the tests), can sort out other tweaks and optimizations.

Thanks for reviewing!

[anomiex]

I think we both think either version of the code is good enough and we'd both defer to the author's preference in this situation. 🙂

Personally I don't mind the separate setting of $srcLength in both code paths, since only one will run and moving it out without potentially setting it unnecessarily would mean having to duplicate the if ($showSources === true) { check. Then again, if I were writing it entirely myself I'd probably not have $srcLength at all as I'd find inlining the strlen call into the expressions easier to follow.

Getting further off topic: I've always found judging cognitive load tricky, I've had plenty of experience both with code I find easy to understand that others tell me they find hard and code I find hard to follow that others tell me they find easier. I've never been able to get a grasp on to what extent that's because I personally think differently from most people and to what extent it's that everyone thinks differently from others.

[jrfnl]

Rebased & squashed without further changes. I will merge this once the build has passed.