Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CodeQuality] Add concat support on ReturnTypeFromStrictScalarReturnExprRector to be string #3435

Merged
merged 16 commits into from
Mar 2, 2023
Merged

[CodeQuality] Add concat support on ReturnTypeFromStrictScalarReturnExprRector to be string #3435

merged 16 commits into from
Mar 2, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Mar 2, 2023 •
edited
Loading

direct return concat will always be string, ref https://3v4l.org/mfs5l unless defined.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner March 2, 2023 07:45
@samsonasik
Copy link
Member Author

samsonasik commented Mar 2, 2023 •
edited
Loading

For combine of integer type, I think that need to be skipped as possibly used as float.

@samsonasik samsonasik force-pushed the concat-support-return-strict branch from d621c90 to e76a1dd Compare March 2, 2023 08:06
@samsonasik samsonasik force-pushed the concat-support-return-strict branch from cf5b05e to 7596dca Compare March 2, 2023 08:15
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik
Copy link
Member Author

samsonasik commented Mar 2, 2023 •
edited
Loading

After some check, concat actually always be string, even object concated:

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@TomasVotruba TomasVotruba merged commit 4e625d0 into main Mar 2, 2023
@TomasVotruba
Copy link
Member

Thank you 👍

@TomasVotruba TomasVotruba deleted the concat-support-return-strict branch March 2, 2023 12:12
staabm
staabm reviewed Mar 5, 2023
View reviewed changes
...ClassMethod/ReturnTypeFromStrictScalarReturnExprRector/Fixture/return_concat_all_int.php.inc
{
public function run()
{
return 1 . 1;
Copy link
Contributor

@staabm staabm Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also be float 1.1 (meaning the . is not a concat op, but a decimal separator )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.1 is float, but 1 . 1 is concat;

ref https://3v4l.org/pFIUa

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow. didn‘t realize whitespace is significant in this case. Makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems suprising enough, that we should have a separate unit case for it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not needed, 1.1 is exactly Dnumber :), 1 . 1 is Concat

samsonasik added a commit that referenced this pull request May 8, 2023
@samsonasik @actions-user
[CodeQuality] Add concat support on ReturnTypeFromStrictScalarReturnE…
98fd46e 
…xprRector to be string (#3435)

Co-authored-by: GitHub Action <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm left review comments

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samsonasik @TomasVotruba @staabm @actions-user