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.

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

Report by issues grouped by level and type #8774

Merged
merged 18 commits into from Dec 2, 2022

Conversation

bdsl
Copy link
Contributor

@bdsl bdsl commented Nov 26, 2022

#8746

This adds a new output format, --output-format=by-issue-level, which is almost exactly the same as the default console output, except that:

  • Issues are sorted by error level then issue type, rather than by file and position in file
  • Error level number is included in output, e.g. ERROR (3): PossiblyNullArgument

This is intended to be useful for getting an overview of a project, particularly when onboarding Psalm to a new codebase, and for choosing what error level to set in psalm config. The most severe errors (plus any reported due to feature specific config) appear first in the output, with the issues that would only be reported at error level 1 appearing at the end.

I wasn't sure where to document this, so for now I've made the report include a block of explanation as a heading. Happy to move that text to a more appropriate place.

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Nov 26, 2022
Comment on lines 162 to 177
$leftLevel = $left->error_level;
$rightLevel = $right->error_level;

if ($leftLevel != $rightLevel) {
if ($rightLevel > 0 && $leftLevel > 0) {
return $rightLevel <=> $leftLevel;
}

if ($rightLevel > 0) {
return -1;
}

return $leftLevel <=> $rightLevel;
}

return $left->type <=> $right->type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't it equivalent to return [$left->error_level, $left->type] <=> [$right->error_level, $right->type]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right, I didn't realise you can use comparison operators with arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this was almost right, but not quite. complication is that negative levels sort to the top, small positive levels sort to the bottom.

@bdsl bdsl force-pushed the report-by-issue-type-severity branch from 2aebebd to aea1c56 Compare December 1, 2022 22:03
@bdsl bdsl changed the title Report by issue type severity Report by issues grouped by level and type Dec 1, 2022
@bdsl bdsl force-pushed the report-by-issue-type-severity branch from b18d502 to 3bc3f1c Compare December 1, 2022 22:11
@bdsl bdsl marked this pull request as ready for review December 1, 2022 23:35
@bdsl
Copy link
Contributor Author

bdsl commented Dec 1, 2022

I'm not sure why the composer update inside the Code Style Analysis job is failing - I don't think I've changed anything that should affect that.

Comment on lines 35 to 37
| The level at which issue is reported as an error is given in brackets - e.g. |
| `ERROR (2): MissingReturnType` indicates that MissingReturnType is only reported |
| as an error when Psalm's level is set to 4 or below. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it level 2 or level 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's level 2, thanks for catching that. Fixed in 5423983.

Comment on lines 47 to 68
ERROR: SomeAlwaysReportedIssueType - file.php:1:1 - message


ERROR: SomeFeatureSpecificIssueType - file.php:1:1 - message


ERROR (7): SomeLevel7IssueType - file.php:1:1 - message


ERROR (4): AnotherLevel4IssueType - file.php:1:1 - message


ERROR (4): SomeLevel4IssueType - file.php:1:1 - message


ERROR (4): SomeLevel4IssueType - file.php:1:1 - message


ERROR (2): SomeLevel2IssueType - file.php:1:1 - message


ERROR (1): SomeIssueType - file.php:1:1 - message
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks a little sparse. Wouldn't it be more readable if we had 1 or two empty lines between levels and none between the issues of the same level?

ERROR: SomeAlwaysReportedIssueType - file.php:1:1 - message
ERROR: SomeFeatureSpecificIssueType - file.php:1:1 - message

ERROR (7): SomeLevel7IssueType - file.php:1:1 - message

ERROR (4): AnotherLevel4IssueType - file.php:1:1 - message
ERROR (4): SomeLevel4IssueType - file.php:1:1 - message
ERROR (4): SomeLevel4IssueType - file.php:1:1 - message

ERROR (2): SomeLevel2IssueType - file.php:1:1 - message

ERROR (1): SomeIssueType - file.php:1:1 - message

Copy link
Contributor Author

@bdsl bdsl Dec 2, 2022

Choose a reason for hiding this comment

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

It's basically the same format as the console report, it just looks sparse in the test because the code snippets are empty. I considered adding a separator between levels but I thought it's probably not worth it. Users should be a be able to do a search for whatever level they're interested in if they want to highlight it in their shell or editor.




EXPECTED, $report->create());
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW now that we require PHP >7.3 you can indent the heredoc terminator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@weirdan
Copy link
Collaborator

weirdan commented Dec 1, 2022

I'm not sure why the composer update inside the Code Style Analysis job is failing

I think we need to add an explicit COMPOSER_ROOT_VERSION=dev-master env variable to composer install / update. Otherwise composer won't be able to guess the root package version and fail the install where there circular dependencies are involved.

weirdan and others added 10 commits December 1, 2022 19:59
I think it makes more sense to have the errors that almost always appear
(level 7 errors) next to the errors that always appear, instead of
the level 1, least likely to appear errors being next to the ones that
always appear.

This also makes the order more similar to that output by the new
--by-issue-level format report.

Some time it might be nice to see if there's a way to auto generate most
of this docs page from the actual issue class definitions, or have
a test that checks the list of issues for each level is accurate and
complete.
…vel & type equal

PHP sorting only became stable in 8.0. For previous versions we would
still like duplicate issues to be sorted into a logical order.
@weirdan weirdan merged commit 4defa17 into vimeo:master Dec 2, 2022
@weirdan
Copy link
Collaborator

weirdan commented Dec 12, 2022

@bdsl the docs/running_psalm/error_levels.md file is generated by bin/generate_levels_doc.php script, so perhaps you need to update that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants