More static analysis (Psalm related changes) #43
Conversation
alexeyshockov
changed the title
alexeyshockov
force-pushed
the
psalm-fixes
branch
from
2edbce5
to
42a6deb
Compare
| "php-coveralls/php-coveralls": "^2.2", | ||
| "vimeo/psalm": "^3.5" | ||
| "vimeo/psalm": "3.14.*" |
There was a problem hiding this comment.
I fixed Psalm version here, to have more consistent builds over time. It should be upgraded sometimes, of course, but having stable builds is more important (as you correctly pointed).
There was a problem hiding this comment.
I think it might be worth going further and constraining to a specific point release of Psalm (e.g. current release 3.14.2. I don't think it's very unusual for psalm patch level releases to detect errors that the previous release wouldn't have detected.
If we don't constrain it to the patch then someone making a PR may have the build fail because of an issue in code they haven't touched and shouldn't be expected to fix.
|
@BenMorel, could you take a look at the coverage report? It says that the coverage decreased dramatically for NativeCalculator, but there are only few lines changed there... |
|
@alexeyshockov regarding coverage, I've already contacted Coveralls for similar problems, and am waiting for their response. I wouldn't be too picky about coverage right now. |
| @@ -291,6 +293,7 @@ public function exactlyDividedBy($that) : BigDecimal | |||
|
|
|||
| [, $b] = $this->scaleValues($this, $that); | |||
|
|
|||
| /** @var numeric-string $d */ | |||
There was a problem hiding this comment.
Is it possible to use @psalm-var here as well?
There was a problem hiding this comment.
@BenMorel It would be yes. Psalm still passes.
| @@ -182,7 +183,7 @@ private static function floatToString(float $float) : string | |||
| * | |||
| * @psalm-pure | |||
| */ | |||
| protected static function create(... $args) : BigNumber | |||
| protected static function create(... $args) : self | |||
There was a problem hiding this comment.
Did you change it to return self as a matter of coding style, or does Psalm complain for some reason when returning BigNumber?
There was a problem hiding this comment.
Psalm is fine with either. As far as I know it should treat them as synonyms.
|
@alexeyshockov What's the point of using According to the docs, |
|
Hi @alexeyshockov, any update on the points above? |
|
@BenMorel I tried a search and replace of 'numeric-string' to 'string' to see if Psalm complains. It does, 23 times. According to psalm the |
|
@bdsl Thank you for your comments. |
|
I upgraded Psalm to 4.1 and fixed all the Psalm issues in cbdd960. @alexeyshockov I did not use We have a strong requirement on the format of the internal numeric strings used in brick/math, something that cannot be enforced strictly enough by Psalm anyway, so I see little value in adding Please feel free to open a new PR if you see something that can be improved! |
Fixes #41