search

MyIntervals / emogrifier

Converts CSS styles into inline style attributes in your HTML code.
https://www.myintervals.com/emogrifier.php
MIT License
906 stars 153 forks source link

Check the code with psalm #537

Closed oliverklee closed 4 years ago

oliverklee commented 6 years ago

https://github.com/vimeo/psalm

SignpostMarv commented 4 years ago

psalm has identified a set of TestCase::assertInstanceOf() checks as redundant, which renders three test cases redundant:

This is likely because the corresponding methods are flags as @return static and so long as psalm doesn't identify a different return type from these methods, it should be safe to remove the tests.

JakeQZ commented 4 years ago

It's also possibly because we're asserting something that's provably true by static code analysis!

SignpostMarv commented 4 years ago

the question would be whether to leave the assertions in to enforce future behaviour, or remove all three test methods as the static analysis "proves" the appropriate type is in play.

JakeQZ commented 4 years ago

I'd prefer to have more checks than fewer, even if they overlap/duplicate, i.e. keep the assertions.

In a similar vein, I recall a comment about Psalm a few days ago where you mentioned something that was already covered by PHPCS (or -fixer). I'd like to have all the tools cover all the possible issues, regardless of overlap. That way you can be more sure nothing is missed (one tool may be better in different ways at picking up a particular issue than another).

SignpostMarv commented 4 years ago

related query to checking with psalm; whether or not to enable the shepherd plugin to have the type-coverage badge in readme.md

oliverklee commented 4 years ago

psalm has identified a set of TestCase::assertInstanceOf() checks as redundant, which renders three test cases redundant:

@SignpostMarv Do you mean "redundant" as in "the tests only need to be in AbstractHtmlProcessorTest" or as in "the return types do not need to be checked as they are already defined by the return type definition"? Or something else?

SignpostMarv commented 4 years ago

psalm has identified a set of TestCase::assertInstanceOf() checks as redundant, which renders three test cases redundant:

@SignpostMarv Do you mean "redundant" as in "the tests only need to be in AbstractHtmlProcessorTest" or as in "the return types do not need to be checked as they are already defined by the return type definition"? Or something else?

as in the issue type is named RedundantConditionGivenDocblockType.

SignpostMarv commented 4 years ago

we're now at the stage in rebased commits where changes affect more than one file.

JakeQZ commented 4 years ago

we're now at the stage in rebased commits where changes affect more than one file.

OK. The principle to stick to is 'small manageable chunks' - if that happens to involve more than one file that's absolutely fine, as long as it's 'small and manageable' :)

oliverklee commented 4 years ago

We now check the code with Psalm, and we should move more type fixes to separate tickets. Closing.