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

Added rector rule to replace ContentType::isContainer property to method #25

Merged
merged 3 commits into from
Mar 1, 2025
Merged

Added rector rule to replace ContentType::isContainer property to method #25

merged 3 commits into from
Mar 1, 2025

Conversation

wiewiurdp
Copy link
Contributor

@wiewiurdp wiewiurdp commented Feb 25, 2025 •
edited
Loading

This was referenced Feb 25, 2025
Merged
Merged
Merged
alongosz
alongosz reviewed Feb 26, 2025
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Can you add test coverage? There's already test for the entire set, it should be as simple as adding a fixture.

@wiewiurdp wiewiurdp requested a review from a team February 27, 2025 11:03
alongosz
alongosz reviewed Feb 27, 2025
View reviewed changes
tests/lib/Sets/Ibexa50/Fixture/change_propoerty_to_method_ContentType.php.inc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Naming: I'd include isContainer reference in the name.
Side note: typo in propoerty ;)

tests/lib/Sets/Ibexa50/Fixture/change_propoerty_to_method_ContentTypeDraft.php.inc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The same remark as above.

tests/lib/Sets/Ibexa50/Fixture/change_propoerty_to_method_Contracts_ContentTypeDraft.php.inc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

naming as well

tests/lib/Sets/Ibexa50/Fixture/rename_class_field.php.inc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this name more descriptive, like rename_content_to_common_namespace or sth similar. It took me 4 looks to see the difference :D :D

@alongosz alongosz requested a review from a team February 27, 2025 11:56
@adamwojs adamwojs merged commit 0f0bc5b into main Mar 1, 2025
3 checks passed
@adamwojs adamwojs deleted the adding-rector-rule-refactoring-ContentType-isContainer-property-to-method branch March 1, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

@alongosz alongosz alongosz approved these changes

@konradoboza konradoboza konradoboza approved these changes

Assignees
No one assigned
Labels
Ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wiewiurdp @adamwojs @alongosz @konradoboza