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.

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

[CodingStyle] Skip concat on first arg on ConsistentImplodeRector #3702

Merged
merged 3 commits into from May 2, 2023

Conversation

nerones
Copy link
Contributor

@nerones nerones commented Apr 28, 2023

Failing Test for ConsistentImplodeRector

Based on https://getrector.com/demo/ec054f63-0f4e-489d-a3da-fa87581814f0

Reported in rectorphp/rector#7903
Closes rectorphp/rector#7903

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Could you try provide a patch as well, the first arg value should check its type insted of value on this line:

if ($firstArgumentValue instanceof String_) {
return null;
}

should can be checked by something like this:

$type = $this->getType($firstArgumentValue);
if ($type->isString()->yes()) {
     return null;
}

@nerones
Copy link
Contributor Author

nerones commented May 1, 2023

Thanks for making it so easy to contribute!

@samsonasik
Copy link
Member

Please update PR title to something like:

[CodingStyle] Skip concat on first arg on ConsistentImplodeRector


namespace Rector\Tests\CodingStyle\Rector\FuncCall\ConsistentImplodeRector\Fixture;

final class DemoFile
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final class DemoFile
final class SkipConcatOnFirstArg

@@ -0,0 +1,13 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

Please update fixture file to : skip_concat_on_first_arg.php.inc

@nerones nerones changed the title Add failing test fixture for ConsistentImplodeRector [CodingStyle] Skip concat on first arg on ConsistentImplodeRector May 2, 2023
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Thank you @nerones

@samsonasik samsonasik merged commit 05cc9af into rectorphp:main May 2, 2023
40 checks passed
@nerones nerones deleted the patch-1 branch May 2, 2023 01:57
samsonasik pushed a commit that referenced this pull request May 8, 2023
)

* Add failing test fixture for ConsistentImplodeRector

# Failing Test for ConsistentImplodeRector

Based on https://getrector.com/demo/ec054f63-0f4e-489d-a3da-fa87581814f0

Reported in rectorphp/rector#7903

* Use the type to check for string in ConsistentImplodeRector

* Change fixture name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants