-
-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent exceptions on UnionTypes #2
Prevent exceptions on UnionTypes #2
Conversation
Good to see someone actually used this package 🥳 I'll look at it in the evening, thanks for the contribution anyway! |
191aa80
to
2f305fd
Compare
@pascalheidmann I've rebased your branch on top of |
2f305fd
to
8f0d9c5
Compare
8f0d9c5
to
b11d3bf
Compare
9fa6dcf
to
0d0f44a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see comment below, did not check proper review option, I'm used to Gitlab more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide ignore test fixture for scenario (like tests/Rule/MultiplyAndDivideByStringRector/Fixture/skip_int_or_string_multiplier_and_divider.php.inc
) which will cover your changes 🙂
0d0f44a
to
7037383
Compare
@Wirone I added a testcase to each rule in which no changes are detected. Hope those fit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for continuing your work 🍻 Even though I find those scenarios weird (PHPStan would complain about them most probably since multiply()
or divide()
could be invoked on non-Money
values) I think this fix is needed 👍
BTW. I just found out that probably I need to change how floats passed directly as an argument are handled - these should be wrapped with sprintf()
too. WDYT?
tests/Rule/MultiplyAndDivideByStringRector/Fixture/skip_union_types.php.inc
Show resolved
Hide resolved
639ff4c
to
0273974
Compare
Thank you @pascalheidmann 🍻 FYI: I've "fixed" errors for PHPStan in main branch, so actions here are green. I'm not fully satisfied with how it was done and I will try to improve it, but your changes can be merged anyway 🙂 |
If you have a codebase with functions that return UnionTypes this rector sadly will fail with a fatal error
This PR won't do magic like checking each type in its union but at least allow the rector to work as intended on simpler code without variable return types.