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 non-empty-string on VarConstantCommentRector #2451

Merged
merged 7 commits into from
Jun 9, 2022

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik requested a review from TomasVotruba as a code owner June 7, 2022 12:24
@samsonasik
Copy link
Member Author

Fixed 🎉 /cc @internalsystemerror

@samsonasik samsonasik force-pushed the skip-non-empty-string branch 2 times, most recently from 8abac3c to 3326934 Compare June 7, 2022 12:27
@internalsystemerror
Copy link
Contributor

@samsonasik Damn that was fast! Thank you!

@samsonasik samsonasik force-pushed the skip-non-empty-string branch from fc6d02a to 96ed6e6 Compare June 7, 2022 12:37
@samsonasik samsonasik force-pushed the skip-non-empty-string branch 2 times, most recently from 02a4a55 to 0253e9f Compare June 7, 2022 13:12
@samsonasik
Copy link
Member Author

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

@@ -92,6 +96,10 @@ public function refactor(Node $node): ?Node
return null;
}

if ($this->shouldSkipStringType($phpDocInfo, $node->consts[0]->value)) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be handled in string type mapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, it previous got @param non-empty-string $expectedName removed on used by other rule, I will check more.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to check on IdentifierTypeMapper aee64c7

Copy link
Member Author

Choose a reason for hiding this comment

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

checking in IdentifierTypeMapper seems cause removal of @param again:

-    /**
-     * @param non-empty-string $function
-     * @param non-empty-string $class
-     * @param non-empty-string $arrayMethod
-     * @param non-empty-string $nonArrayMethod
-     */

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's try register AccessoryNonEmptyStringType into ScalarStringToTypeMapper 831a7e3

Copy link
Member Author

Choose a reason for hiding this comment

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

it works 🎉

Copy link
Member

@TomasVotruba TomasVotruba Jun 9, 2022

Choose a reason for hiding this comment

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

Thank you, that seems the best place 👍

@samsonasik samsonasik force-pushed the skip-non-empty-string branch from 0253e9f to aee64c7 Compare June 9, 2022 00:09
@samsonasik samsonasik force-pushed the skip-non-empty-string branch from 2168d3a to 831a7e3 Compare June 9, 2022 00:24
@samsonasik
Copy link
Member Author

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

@TomasVotruba TomasVotruba merged commit ff931e4 into main Jun 9, 2022
@TomasVotruba TomasVotruba deleted the skip-non-empty-string branch June 9, 2022 06:09
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.

VarConstantCommentRector changes non-empty-string to string
3 participants