Skip to content

Conversation

eigan
Copy link
Contributor

@eigan eigan commented Apr 24, 2020

Not sure if this should be split into multiple extensions, kept it at one because they all had very similar logic.

@eigan eigan force-pushed the bcdiv-dynamic-return branch from b1a9f50 to 8fb2229 Compare April 27, 2020 17:23
@eigan
Copy link
Contributor Author

eigan commented Apr 27, 2020

Well this got way more complicated than what I initially thought.. Might need some more tests too. Noticed that PHP doesnt always return the value as described in doc either.

@eigan eigan force-pushed the bcdiv-dynamic-return branch 2 times, most recently from 554c3a5 to 070cd25 Compare April 27, 2020 17:29
@ondrejmirtes
Copy link
Member

It's never easy :) Keep me posted, thanks. You can run the build locally to get faster feedback too - I'd run vendor/bin/phing cs, tests-fast, and phpstan.

@eigan eigan force-pushed the bcdiv-dynamic-return branch 3 times, most recently from 0742536 to 21281bc Compare May 22, 2020 05:26
@eigan
Copy link
Contributor Author

eigan commented May 22, 2020

I think im done with this now @ondrejmirtes. I have verified my assertions here https://3v4l.org/RWD9r. Some testcases fails, but they seem to be unrelated.

@eigan eigan force-pushed the bcdiv-dynamic-return branch from 21281bc to cb38ee3 Compare May 22, 2020 16:46
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Jul 27, 2020

It'd be nice if someone brought this PR over the finish line and verified that it also handles phpstan/phpstan#3551.

@eigan eigan force-pushed the bcdiv-dynamic-return branch 5 times, most recently from 3910f8c to f25b6b2 Compare July 28, 2020 06:07
@eigan
Copy link
Contributor Author

eigan commented Jul 28, 2020

This PR is ready for another review @ondrejmirtes.

1) PHPStan tests fail. I thought PHPStan would understand the code written?

$secondArgumentIsConstant = $secondArgument instanceof ConstantScalarType;
$secondArgumentIsNumeric = $secondArgumentIsConstant && is_numeric($secondArgument->getValue());
Call to an undefined method PHPStan\Type\Type::getValue()

2) Tests fails on 7.4 and 8.0. Seems to be unrelated.

3) Code is a mess 😞

@eigan eigan force-pushed the bcdiv-dynamic-return branch from f25b6b2 to aa9eb8c Compare September 20, 2020 10:22
@eigan
Copy link
Contributor Author

eigan commented Sep 20, 2020

Rebased on master and refactored the code so the tests passes (mentioned why in last comment).

@ondrejmirtes
Copy link
Member

Please rebase one more time and then I'll merge it, thank you.

@eigan eigan force-pushed the bcdiv-dynamic-return branch 2 times, most recently from d7443ca to 8c07cf2 Compare October 1, 2020 18:13
eigan added 2 commits October 1, 2020 20:18
Some bcmath functions will always return a specific type based on the arguments.
@eigan eigan force-pushed the bcdiv-dynamic-return branch from 8c07cf2 to 7f3f610 Compare October 1, 2020 18:18
@eigan
Copy link
Contributor Author

eigan commented Oct 2, 2020

✔️

@ondrejmirtes ondrejmirtes merged commit 87fc37e into phpstan:master Oct 2, 2020
@ondrejmirtes
Copy link
Member

Thank you!

@eigan eigan deleted the bcdiv-dynamic-return branch October 2, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants