Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Report Full: fix delimiter line bug #154

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Report Full: fix delimiter line bug #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 18 additions & 5 deletions src/Reports/Full.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,28 @@ public function generateFileReport($report, File $phpcsFile, $showSources=false,
foreach ($report['messages'] as $line => $lineErrors) {
foreach ($lineErrors as $column => $colErrors) {
foreach ($colErrors as $error) {
$length = strlen($error['message']);
// Start with the presumption of a single line error message.
$length = strlen($error['message']);
$srcLength = (strlen($error['source']) + 3);
if ($showSources === true) {
$length += (strlen($error['source']) + 3);
$length += $srcLength;
}

// ... but also handle multi-line messages correctly.
if (strpos($error['message'], "\n") !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jrfnl jrfnl Dec 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) 😉.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

$errorLines = explode("\n", $error['message']);
$length = max(array_map('strlen', $errorLines));

if ($showSources === true) {
$lastLine = array_pop($errorLines);
jrfnl marked this conversation as resolved.
Show resolved Hide resolved
$length = max($length, (strlen($lastLine) + $srcLength));
}
}

$maxErrorLength = max($maxErrorLength, ($length + 1));
}
}
}
}//end foreach
}//end foreach
}//end foreach

$file = $report['filename'];
$fileLength = strlen($file);
Expand Down