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

refactor: add CompleteDynamicPropertiesRector #6187

Merged
merged 4 commits into from
Jun 28, 2022
Merged

refactor: add CompleteDynamicPropertiesRector #6187

merged 4 commits into from
Jun 28, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 27, 2022 •
edited
Loading

Description

  • add CompleteDynamicPropertiesRector
  • fix visibility for test class properties

See #6172 (comment)

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the refactor Pull requests that refactor code label Jun 27, 2022
@kenjis kenjis mentioned this pull request Jun 27, 2022
Closed
7 tasks
MGatner
MGatner approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why this only applies to tests/ but it looks good to me. I think 95% of these could all be scoped narrower but since they are test cases I don't really care.

Edit: they are all final classes so 100% could be private.

@kenjis kenjis force-pushed the refactor-CompleteDynamicPropertiesRector branch 2 times, most recently from 24df156 to 52f9407 Compare June 27, 2022 22:20
@kenjis
Copy link
Member Author

kenjis commented Jun 27, 2022

Changed public to private.

@kenjis kenjis force-pushed the refactor-CompleteDynamicPropertiesRector branch from 52f9407 to e49c0bd Compare June 28, 2022 00:11
@kenjis kenjis requested a review from MGatner June 28, 2022 00:32
MGatner
MGatner approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

LGTM! Did you only run Rector on tests/? Or were there really no dynamic property violations in system/?

@kenjis
Copy link
Member Author

kenjis commented Jun 28, 2022

Did you only run Rector on tests/?

No.

@kenjis kenjis merged commit 24f67ea into codeigniter4:develop Jun 28, 2022
@kenjis kenjis deleted the refactor-CompleteDynamicPropertiesRector branch June 28, 2022 11:42
@kenjis kenjis added the PHP 8.2 label Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan approved these changes

@MGatner MGatner MGatner approved these changes

Assignees
No one assigned
Labels
refactor Pull requests that refactor code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kenjis @MGatner @paulbalandan