-
-
Notifications
You must be signed in to change notification settings - Fork 431
[ci] add short class fixture names check to avoid node class names #7548
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
Conversation
fe0a88a to
b4f559b
Compare
b4f559b to
111f057
Compare
| @@ -1,51 +0,0 @@ | |||
| <?php | |||
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.
this fixture test is needed, I can create separate PR for update fixture with new class name to not conflict with Node name :)
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.
Same here. We already have this case covered. I remove these fixtures on purpose, to make tests more readable and maintainable.
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.
no, loop of multi properties is special :), because it already too early imported on class level, I will find a referenced PR :)
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.
Here the issue and PR that it fixed:
- AnnotationToAttributeRector only replace the first annotation when using use statement aliases rector#9114
- [Php80] Handle aliased with multi properties on AnnotationToAttributeRector #6838
the fixture is needed to avoid the patch removed in the future.
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.
@samsonasik It should at least re-use existing configuration. No need to add new configuration item, morever of non-existing class. That makes test less readable.
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.
sure, I will add to use existing config 👍
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.
| @@ -1,36 +0,0 @@ | |||
| <?php | |||
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.
this fixture test is needed, I can create separate PR for update fixture with new class name to not conflict with Node name :)
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.
This is on purpose, not to create fixture per every possible item, but make them readable.
Don't add anything here.
Uh oh!
There was an error while loading. Please reload this page.