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

[php] Configure Psalm 6 default settings findUnusedBaselineEntry, findUnusedCode #234

Merged

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Mar 12, 2024

🤔 What's changed?

Psalm's offering news, and our CI had not configured those settings.

This PR fixes a warning that outputs that we should be explicit about this setting.

First, I picked enabling it as the solution. (And, then I continued on that path.)

Psalm in CI: found some issues: 9 errors found

░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
Error: src/AstNode.php:38:37: UnusedParam: Param #1 is never referenced in this method (see https://psalm.dev/135)
Error: src/GherkinParser.php:31:31: UnusedProperty: Cannot find any references to private property Cucumber\Gherkin\GherkinParser::$predictableIds (see https://psalm.dev/150)
Error: src/GherkinParser.php:46:21: PossiblyUnusedMethod: Cannot find any calls to method Cucumber\Gherkin\GherkinParser::parseString (see https://psalm.dev/087)
Error: src/Location.php:15:21: PossiblyUnusedMethod: Cannot find any calls to method Cucumber\Gherkin\Location::getColumn (see https://psalm.dev/087)
Error: src/Parser/ParserTrait.php:76:16: UnusedReturnValue: The return value for this private method is never used (see https://psalm.dev/272)
Error: src/ParserException/UnexpectedEofException.php:16:31: PossiblyUnusedProperty: Cannot find any references to property Cucumber\Gherkin\ParserException\UnexpectedEofException::$receivedToken (see https://psalm.dev/149)
Error: src/ParserException/UnexpectedEofException.php:17:31: PossiblyUnusedProperty: Cannot find any references to property Cucumber\Gherkin\ParserException\UnexpectedEofException::$expectedTokenTypes (see https://psalm.dev/149)
Error: src/ParserException/UnexpectedEofException.php:18:32: PossiblyUnusedProperty: Cannot find any references to property Cucumber\Gherkin\ParserException\UnexpectedEofException::$stateComment (see https://psalm.dev/149)
Error: src/ParserException/UnexpectedTokenException.php:19:32: PossiblyUnusedProperty: Cannot find any references to property Cucumber\Gherkin\ParserException\UnexpectedTokenException::$stateComment (see https://psalm.dev/149)
------------------------------
9 errors found
------------------------------
Psalm can automatically fix 3 of these issues.
Run Psalm again with 
--alter --issues=PossiblyUnusedMethod,MissingParamType --dry-run
to see what it can fix.
------------------------------

Checks took 3.46 seconds and used 110.339MB of memory
Psalm was able to infer types for 100% of the codebase
Error: Process completed with exit code 2.

OK, so perhaps "enable" wasn't the right way, let's see if disabling will lead to nicer output? I learned a little, and placed some Psalm annotations in the code, in order to tell Psalm "these properties on the exceptions, they're expected, across all our implementations".

That got us a shorter list of Psalm offenses.

Psalm in CI: 5 errors found

░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
Error: src/AstNode.php:38:37: UnusedParam: Param #1 is never referenced in this method (see https://psalm.dev/135)
Error: src/GherkinParser.php:31:31: UnusedProperty: Cannot find any references to private property Cucumber\Gherkin\GherkinParser::$predictableIds (see https://psalm.dev/150)
Error: src/GherkinParser.php:46:21: PossiblyUnusedMethod: Cannot find any calls to method Cucumber\Gherkin\GherkinParser::parseString (see https://psalm.dev/087)
Error: src/Location.php:15:21: PossiblyUnusedMethod: Cannot find any calls to method Cucumber\Gherkin\Location::getColumn (see https://psalm.dev/087)
Error: src/Parser/ParserTrait.php:76:16: UnusedReturnValue: The return value for this private method is never used (see https://psalm.dev/272)
------------------------------
5 errors found
------------------------------

Upon thinking about those issues reported, I sort of think that it can absolutely be that it's part of the API to have some unused parameters (in code like AST handling callbacks). So, perhaps they're all @psalm-api.

⚡️ What's your motivation?

CI output should be terse, clear and focused. This change hopes to make it less noisy.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)

  • I agree to respect and uphold the Cucumber Community Code of Conduct

This fixes a warning that outputs that we should be explicit about this setting. I picked enabling it as the solution.
@olleolleolle olleolleolle changed the title [php] Configure Psalm 6 default settings [php] Configure Psalm 6 default settings findUnusedBaselineEntry, findUnusedCode Mar 12, 2024
We were not ready yet. These are now explicitly disabled, which takes care of the reminder message.
This tells Psalm that these are a part of an expected API.
@olleolleolle olleolleolle marked this pull request as draft March 12, 2024 13:58
php/psalm.xml Outdated
@@ -2,6 +2,8 @@
<psalm
errorLevel="1"
resolveFromConfigFile="true"
findUnusedBaselineEntry="true"
findUnusedCode="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put these on to false. To get some warnings out of the way and then tackle what is left piece meal. Especially in the generated code it will be hard to follow psalms recommendations.

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 did put them to false, which didn't change the output. That confused me, so I continued on the track "follow the suggestions".

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird. I can't reproduce that locally. Pushed a commit to see if it matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpkorstanje This is super! Thanks for bringing it over the line. A lot less output!

* Run `vendor/bin/psalm --no-cache --alter --issues=MissingParamType`
* Bump "phpunit/phpunit": "^10.5" to minimum deps run doesn't use a version that is too old
* Upgrade to latest phpunit.xml format
* Set findUnusedBaselineEntry="false" and findUnusedCode="false" to suppress paslm warnings
@olleolleolle olleolleolle marked this pull request as ready for review March 14, 2024 18:52
@olleolleolle olleolleolle merged commit a189ff0 into cucumber:main Mar 14, 2024
3 checks passed
@olleolleolle olleolleolle deleted the php-psalm-configure-missing-settings branch March 14, 2024 18:53
@olleolleolle
Copy link
Contributor Author

There!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants