search

brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

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

Closed bdsl closed 4 years ago

bdsl commented 4 years ago

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

BenMorel commented 4 years ago

Hi, thanks for your PR! I'm waiting for https://github.com/brick/math/pull/43 to be merged, then I'll come back to you 👍

bdsl commented 4 years ago

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.

bdsl commented 4 years ago

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

BenMorel commented 4 years ago

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 https://github.com/brick/math/pull/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 https://github.com/brick/math/pull/43!

bdsl commented 4 years ago

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 commented 4 years ago

@BenMorel Do you want to take another look at this now that https://github.com/brick/math/pull/43#issuecomment-719548336 is closed and Psalm is working in brick/math ?

BenMorel commented 4 years ago

@bdsl Please check my comments, and it will be good to go! :+1:

bdsl commented 4 years ago

@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 commented 4 years ago

Thanks, @bdsl!