Add static analysis using Psalm, and fix reported issues #26

bdsl · 2020-08-02T01:07:52Z

This is using the second strictest set of checks in Psalm default
configs. It might be interesting to get the library working on
the strictest level, which would mean resolving all the issues with
mixed types

psalm.xml

jdreesen · 2020-08-02T09:43:02Z

composer.json

@@ -9,7 +9,8 @@
    ],
    "license": "MIT",
    "require": {
-        "php": "^7.1|^8.0"
+        "php": "^7.1|^8.0",
+        "psalm/phar": "^3.13"


Psalm should be a dev dependency.

Thanks, you're right of course. Having it in require would cause conflicts with projects that use this library and require a different version of psalm/phar. Fixed in b4f457b

BenMorel · 2020-08-26T19:01:07Z

Hi, thanks for your PR! I'm waiting for brick/math#43 to be merged, then I'll come back to you 👍

bdsl · 2020-08-26T19:48:23Z

Hi Ben. I hadn't seen brick/math#43 . Is there are a dependency between that and this PR? Or you want to learn from that one before you decide what what to do here?

Let me know if you'd like me to rebase this on the latest master.

If you think it would be useful I'd be up for adding more checking tools like PHPStan or perhaps Infection, and maybe increasing the Psalm strictness level and making it cover tests as well as source.

src/LocalDateRange.php

src/YearMonthRange.php

This is using the second strictest set of checks in Psalm default configs. It might be interesting to get the library working on the strictest level, which would mean resolving all the issues with mixed types

bdsl · 2020-08-26T19:58:59Z

I went ahead and rebased on latest master ( 3d29c70 ) to make sure it still passes.

BenMorel · 2020-08-26T20:38:18Z

Or you want to learn from that one before you decide what what to do here?

Exactly.

If you think it would be useful I'd be up for adding more checking tools like PHPStan or perhaps Infection, and maybe increasing the Psalm strictness level and making it cover tests as well as source.

+1 for maximum strictness, if that doesn’t affect legitimate code (example: in the other PR, adding a method was suggested, just to make Psalm happy - I’m not OK with that).

I’m still not sure if I want to use both Psalm and phpstan (what about phan?), I’ll be happy to hear your thoughts about this.

I’m also willing to add a coding style, with phpcs and/or php-cs-fixer (see brick/math#44). Infection, why not. But I’ll be using brick/math (my most popular library) as a testing ground before deploying this to all my libraries, to try and keep them consistent with each other!

I appreciate to have 2 separate contributions for code quality though, and would be grateful if you can participate to brick/math#43!

bdsl · 2020-08-26T20:52:02Z

Ok, I'll try and find time to take a good look at brick/math#43 soon and I can try and answer some of your questions about it if @alexeyshockov hasn't answered them first, and maybe also join the discussion about the coding style tool over at brick/math.

About phpstan, I think it has several types of issues it can catch that Psalm doesn't, and vice versa, so you find the most possible issues by using both. I only have experience with Psalm personally.

bdsl · 2020-10-30T15:38:12Z

@BenMorel Do you want to take another look at this now that brick/math#43 (comment) is closed and Psalm is working in brick/math ?

BenMorel · 2020-11-09T09:38:32Z

src/ZonedDateTime.php

+         *
+         * Looks like Psalm might have a wrong return type for \DateTimeInterface::getTimeZone - it should
+         * be \DateTimeZone|false . Psalm seems to think it's just \DateTimeZone
+         */


Reported here: vimeo/psalm#4515
Please add a link in the comment!

BenMorel · 2020-11-09T09:38:47Z

src/Duration.php


        if ($nanoseconds < 0) {
            $nanoseconds += LocalTime::NANOS_PER_SECOND;
            $seconds--;
        }

+


Please remove this extra line

BenMorel · 2020-11-09T09:39:29Z

composer.json

@@ -13,7 +13,8 @@
    },
    "require-dev": {
        "phpunit/phpunit": "^7.5.15",
-        "php-coveralls/php-coveralls": "^2.2"
+        "php-coveralls/php-coveralls": "^2.2",
+        "psalm/phar": "^3.13"


Could you please upgrade to Psalm ^4.1?

BenMorel · 2020-11-09T09:40:44Z

.travis.yml

@@ -11,6 +11,7 @@ before_script:

 script:
  - mkdir -p build/logs
+  - vendor/bin/psalm.phar


Instead of running Psalm with every build, it would be nice to create a separate job for it.
You can take inspiration from brick/math: https://github.com/brick/math/blob/master/.travis.yml

I'm not clear on what the benefit of the separate job would be at this stage. Psalm only seems to take 1.15 seconds to run, so the overheads of setting up a separate job probably wouldn't be justified by the potential savings of running it in parallel with other things. I'm also not very familiar with Travis.

You have a point here, though it's more of a matter of proper job separation, so that when a job fails, you know that it's because of a failed test and not because of a Psalm issue. This also gives the ability to use Psalm as one of the "smoke tests" that prevent other tests from running if it doesn't pass. Let's merge it this way for now, I'll come back to this later.

BenMorel · 2020-11-09T09:42:48Z

@bdsl Please check my comments, and it will be good to go! 👍

bdsl · 2020-11-11T17:53:19Z

@BenMorel Thanks, I've applied all your suggestions apart from the one about travis. I was going to consider that next, but maybe Travis is having problems - the job for building 0777a78 seems to have been queued for several minutes. I'm not sure how long it should normally take. I'll take another look later and see if it's run.

BenMorel · 2020-11-13T10:53:59Z

Thanks, @bdsl!

bdsl mentioned this pull request Aug 2, 2020

Check code with static analysis tool(s) #27

Closed

bdsl commented Aug 2, 2020

View reviewed changes

psalm.xml Show resolved Hide resolved

jdreesen reviewed Aug 2, 2020

View reviewed changes

bdsl commented Aug 26, 2020

View reviewed changes

src/LocalDateRange.php Show resolved Hide resolved

bdsl commented Aug 26, 2020

View reviewed changes

src/YearMonthRange.php Show resolved Hide resolved

bdsl force-pushed the psalm branch from eaac178 to b4f457b Compare August 26, 2020 19:56

bdsl added 2 commits August 26, 2020 20:56

Add static analysis using Psalm, and fix reported issues

2104ae7

This is using the second strictest set of checks in Psalm default configs. It might be interesting to get the library working on the strictest level, which would mean resolving all the issues with mixed types

Require Psalm only in dev

5219eed

bdsl force-pushed the psalm branch from b4f457b to 5219eed Compare August 26, 2020 19:57

BenMorel reviewed Nov 9, 2020

View reviewed changes

bdsl added 2 commits November 11, 2020 17:29

Add link to Psalm issue

fd52bf4

Remove blank line

787ac9f

Update to latest Psalm: 4.1.1

40f8fb1

bdsl force-pushed the psalm branch from 0777a78 to 40f8fb1 Compare November 11, 2020 18:01

Fix typo in psalm-suppress annotation

ec69347

BenMorel merged commit 6a828b4 into brick:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static analysis using Psalm, and fix reported issues #26

Add static analysis using Psalm, and fix reported issues #26

bdsl commented Aug 2, 2020

jdreesen Aug 2, 2020

bdsl Aug 2, 2020

BenMorel commented Aug 26, 2020 •

edited

Loading

bdsl commented Aug 26, 2020

bdsl commented Aug 26, 2020

BenMorel commented Aug 26, 2020 •

edited

Loading

bdsl commented Aug 26, 2020 •

edited

Loading

bdsl commented Oct 30, 2020

BenMorel Nov 9, 2020

bdsl Nov 12, 2020

BenMorel Nov 9, 2020

bdsl Nov 12, 2020

BenMorel Nov 9, 2020

bdsl Nov 12, 2020

BenMorel Nov 9, 2020

bdsl Nov 12, 2020

BenMorel Nov 13, 2020

BenMorel commented Nov 9, 2020

bdsl commented Nov 11, 2020

BenMorel commented Nov 13, 2020

Add static analysis using Psalm, and fix reported issues #26

Add static analysis using Psalm, and fix reported issues #26

Conversation

bdsl commented Aug 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel commented Aug 26, 2020 • edited Loading

bdsl commented Aug 26, 2020

bdsl commented Aug 26, 2020

BenMorel commented Aug 26, 2020 • edited Loading

bdsl commented Aug 26, 2020 • edited Loading

bdsl commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorel commented Nov 9, 2020

bdsl commented Nov 11, 2020

BenMorel commented Nov 13, 2020

BenMorel commented Aug 26, 2020 •

edited

Loading

BenMorel commented Aug 26, 2020 •

edited

Loading

bdsl commented Aug 26, 2020 •

edited

Loading