[TASK] Check the code with psalm #789

SignpostMarv · 2019-10-03T17:20:00Z

separate PR made as per discussion in #778

Psalm's default config ships with a plethora of common issues that're set to `errorLevel="info"` `psalm --show-info=false` avoids displaying such issues in cli/ ci logs regarding: MyIntervals#537

Since other CI tasks check additional directories, ci:php:psalm should. regarding: MyIntervals#537

Prior to Psalm 3.x issues that were not suppressed by `errorLevel="info"` + `psalm --show-info=false` could be suppressed via the `@psalm-suppress` docblock tag. The downside of this approach being that suppressed issues may be forgotten about. Baseline files allow issues to be suppressed en-masse, then selectively fixed at a later date. regarding: MyIntervals#537

PHPUnit test cases can cause psalm to identify false positives, i.e. thinking that a dataProvider or test method is unused. The psalm/plugin-phpunit package resolves some of these false positives and also helps identify additional issues. regarding: MyIntervals#537

This commit updates the psalm baseline separately from enabling the plugin to allow `composer run ci` to run succesfully from this point. regarding: MyIntervals#537

SignpostMarv · 2019-10-03T17:20:41Z

this will probably fail because of vimeo/psalm#1192

This commit manually amends the directory seperators in the psalm baseline, as the current dependency constraints prevent a later release of Psalm from being used. regarding: vimeo/psalm#1192, MyIntervals#789

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

regarding: MyIntervals#778

This commit manually amends the directory seperators in the psalm baseline, as the current dependency constraints prevent a later release of Psalm from being used. regarding: vimeo/psalm#1192, MyIntervals#789

This commit raises the minimum version of psalm to the latest release of 3.2, as the format of some baselined issues changed between 3.2 and 3.2.12 (causing travis to succeed in --prefer-lowest). regarding: https://travis-ci.org/MyIntervals/emogrifier/jobs/593163215

oliverklee

I've added a few comments. (I've also moved the entry in the changelog to the "added" section and fixed the PR number.)

composer.json

oliverklee · 2019-10-03T20:59:37Z

.travis.yml

+- >
+  echo;
+  echo "Running Psalm";
+  composer ci:php:psalm;


I propose we run this before phpcs and php-cs-fixer (type checks before style) so the more functional failures will come first.

Generally I'd say yes, but I've found that php-cs-fixer will break some indented array shape docblock values.

Would defer this until stage two of psalm checking (all issues resolved) is complete or #742 is resolved (since array shapes are used during static analysis w/ psalm)

We're running php-cs-fixer in dry-run mode, i.e., it won't change any code.

We're running php-cs-fixer in dry-run mode, i.e., it won't change any code.

do you manually fix issues identified by php-cs-fixer?

do you manually fix issues identified by php-cs-fixer?

Yes (in the sense that we'll need to commit those changes so the build will be green, and that there's no bot amending the failing PRs).

Travis-CI rejects commits that cause issues. I know that @oliverklee has used the fixer automatically, both before we had this check, and as we introduced it, but locally. (I could find a reference, but I've just used up all my refrence-finding time for this month.)

Having a local composer ci script to do it could be useful, but perhaps mostly only if we're introducing new checks (or upgrading to a new version that does). On the backburner maybe, though if it's something you'd like to aid development, please submit a PR :)

(Personally, I'd rather make the suggested edits manually, if it's just a new feature I'm indroducing and I've fallen foul of some rules, so I can be clear where I'm at.)

do you manually fix issues identified by php-cs-fixer?

Yes (in the sense that we'll need to commit those changes so the build will be green, and that there's no bot amending the failing PRs).

my point being that if php-cs-fixer --fix makes larges swathes of changes, indented psalm array shapes being removed can be inadvertently overlooked & committed (the workaround is to un-indent/nest the array shape from multi-line to a single line).

I'm still not sure I get your point. What's the exact problem you see, or what do you suggest?

I occasionally run psalm after php-cs-fixer on the basis that php-cs-fixer may identify an issue, so I take out the --dry-run, which may then cause an issue in psalm.

tl:dr; either order is fine, caveat coder.

psalm.xml

This commit removes the default issue handlers from the psalm config, and updates the psalm baseline accordingly. regarding: https://github.com/MyIntervals/emogrifier/pull/789/files/c8dc0922cf4ec03ee218110ab37233dabb2c531b#r331250918

This commit enables psalm's totallyTyped flag, then updates the psalm baseline accordingly. > Enabling this will make Psalm very strict, such that it needs to be > able to evaluate the type of every single statement > > https://psalm.dev/docs/running_psalm/configuration/ regarding: https://github.com/MyIntervals/emogrifier/pull/789/files/c8dc0922cf4ec03ee218110ab37233dabb2c531b#r331385117

This commit enables psalm's strictBinaryOperands flag, then updates the psalm baseline accordingly. > force strict typing on numerical and string operations > > https://psalm.dev/docs/running_psalm/configuration/ regarding: MyIntervals#789 (comment)

Since literal void return types cannot yet be used, and decorating all affected methods with `@return void` may cause annoyance, this commit removes the requirement to have void returns flagged appropriately. regarding: MyIntervals#793, MyIntervals#789 (comment)

This commit enable's psalm's dead code detection, then updates the psalm baseline accordingly. > When true, Psalm will attempt to find all unused code > (including unused variables), > the equivalent of running with --find-unused-code. > > https://psalm.dev/docs/running_psalm/configuration/ regarding: MyIntervals#789 (comment)

This commit adjusts the local XSD path to match that used in later releases of psalm. This issue will regress and require manual attention until a later release of psalm can be used. regarding: MyIntervals#793

oliverklee

Looks very good. If you change the order of the steps in .travis.yml, I can merge this PR.

JakeQZ · 2019-10-04T22:41:43Z

Looks very good. If you change the order of the steps in .travis.yml, I can merge this PR.

I agree. We can perhaps add some separate 'cleanup' issues to be addressed after this and also #778 are merged, that possibly arise from the discussion in #778.

This commit changes the order of task execution, psalm was added to travis previously in 40201a3 regarding: MyIntervals#790, MyIntervals#789 (review)

oliverklee

Looks good. Thanks!

SignpostMarv added 5 commits October 2, 2019 19:06

[TASK] add psalm with default config

886ec1c

Psalm's default config ships with a plethora of common issues that're set to `errorLevel="info"` `psalm --show-info=false` avoids displaying such issues in cli/ ci logs regarding: MyIntervals#537

[TASK] have psalm check config & tests directories

cf58550

Since other CI tasks check additional directories, ci:php:psalm should. regarding: MyIntervals#537

[TASK] updating psalm baseline

21707e2

This commit updates the psalm baseline separately from enabling the plugin to allow `composer run ci` to run succesfully from this point. regarding: MyIntervals#537

SignpostMarv added 3 commits October 3, 2019 18:30

[TASK] Check the code with psalm

be8e768

This commit updates the changelog in reference to the previous commits implementing psalm checking & resolving issues. regarding: MyIntervals#537, MyIntervals#778

[TASK] run psalm on travis

40201a3

regarding: MyIntervals#778

[BUGFIX] amend directory separators in baseline

130782e

This commit manually amends the directory seperators in the psalm baseline, as the current dependency constraints prevent a later release of Psalm from being used. regarding: vimeo/psalm#1192, MyIntervals#789

SignpostMarv force-pushed the psalm-part-one branch from 74e18a3 to 130782e Compare October 3, 2019 17:33

SignpostMarv and others added 2 commits October 3, 2019 18:44

Update CHANGELOG.md

c8dc092

oliverklee requested changes Oct 3, 2019

View reviewed changes

oliverklee assigned SignpostMarv Oct 3, 2019

oliverklee added the enhancement label Oct 3, 2019

oliverklee added this to the 4.0.0 milestone Oct 3, 2019

SignpostMarv mentioned this pull request Oct 4, 2019

Psalm #778

Closed

SignpostMarv added 6 commits October 4, 2019 18:28

[CLEANUP] remove default psalm issue handlers

013095e

This commit removes the default issue handlers from the psalm config, and updates the psalm baseline accordingly. regarding: https://github.com/MyIntervals/emogrifier/pull/789/files/c8dc0922cf4ec03ee218110ab37233dabb2c531b#r331250918

[TASK] Check the code with psalm

9e4234d

This commit enables psalm's strictBinaryOperands flag, then updates the psalm baseline accordingly. > force strict typing on numerical and string operations > > https://psalm.dev/docs/running_psalm/configuration/ regarding: MyIntervals#789 (comment)

[CLEANUP] normalise absolute path to relative path

2b6ec42

This commit adjusts the local XSD path to match that used in later releases of psalm. This issue will regress and require manual attention until a later release of psalm can be used. regarding: MyIntervals#793

SignpostMarv mentioned this pull request Oct 4, 2019

Add Psalm to Travis #790

Closed

oliverklee requested changes Oct 4, 2019

View reviewed changes

[TASK] Add Psalm to Travis

6b6205c

This commit changes the order of task execution, psalm was added to travis previously in 40201a3 regarding: MyIntervals#790, MyIntervals#789 (review)

oliverklee approved these changes Oct 5, 2019

View reviewed changes

oliverklee merged commit b8fa86b into MyIntervals:master Oct 5, 2019

SignpostMarv deleted the psalm-part-one branch October 5, 2019 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TASK] Check the code with psalm #789

[TASK] Check the code with psalm #789

SignpostMarv commented Oct 3, 2019

SignpostMarv commented Oct 3, 2019

oliverklee left a comment

oliverklee Oct 3, 2019

SignpostMarv Oct 4, 2019 •

edited

Loading

oliverklee Oct 4, 2019

SignpostMarv Oct 5, 2019

oliverklee Oct 5, 2019

JakeQZ Oct 6, 2019 •

edited

Loading

SignpostMarv Oct 6, 2019

oliverklee Oct 6, 2019

SignpostMarv Oct 6, 2019

oliverklee left a comment

JakeQZ commented Oct 4, 2019 •

edited

Loading

oliverklee left a comment

[TASK] Check the code with psalm #789

[TASK] Check the code with psalm #789

Conversation

SignpostMarv commented Oct 3, 2019

SignpostMarv commented Oct 3, 2019

oliverklee left a comment

Choose a reason for hiding this comment

oliverklee Oct 3, 2019

Choose a reason for hiding this comment

SignpostMarv Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

oliverklee Oct 4, 2019

Choose a reason for hiding this comment

SignpostMarv Oct 5, 2019

Choose a reason for hiding this comment

oliverklee Oct 5, 2019

Choose a reason for hiding this comment

JakeQZ Oct 6, 2019 • edited Loading

Choose a reason for hiding this comment

SignpostMarv Oct 6, 2019

Choose a reason for hiding this comment

oliverklee Oct 6, 2019

Choose a reason for hiding this comment

SignpostMarv Oct 6, 2019

Choose a reason for hiding this comment

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ commented Oct 4, 2019 • edited Loading

oliverklee left a comment

Choose a reason for hiding this comment

SignpostMarv Oct 4, 2019 •

edited

Loading

JakeQZ Oct 6, 2019 •

edited

Loading

JakeQZ commented Oct 4, 2019 •

edited

Loading