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

[Privatization] Skip variable assign append on ChangeReadOnlyVariableWithDefaultValueToConstantRector #3687

Merged
merged 6 commits into from Apr 27, 2023

Conversation

mickeytodd
Copy link
Contributor

@mickeytodd mickeytodd commented Apr 25, 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.

Please add fixture test under

https://github.com/rectorphp/rector-src/tree/main/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture

If it going to skip the change, you can copy one of fixture file with skip_ prefix and modify

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.

Please rename PR title to something like:

[Privatization]  Skip variable assign append on ChangeReadOnlyVariableWithDefaultValueToConstantRector

Also, could you make CI green, see error

https://github.com/rectorphp/rector-src/actions/runs/4803271285/jobs/8547601933?pr=3687#step:5:18

@mickeytodd mickeytodd changed the title Bugfix for issue #7900 [Privatization] Skip variable assign append on ChangeReadOnlyVariableWithDefaultValueToConstantRector Apr 26, 2023
@mickeytodd
Copy link
Contributor Author

@samsonasik could you please check why testing fails on Renaming/Rector/FileWithoutNamespace/PseudoNamespaceToNamespaceRector/Fixture/namespace_split.php.inc? I didn't change that file. Thanks!

@samsonasik
Copy link
Member

I am not sure, could you try rebase? If still happen, try revert your change, if still happen, it possibly due to duplicated fixture files

@samsonasik
Copy link
Member

Test failure fixed at #3691, please rebase

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.

Looks good, thank you 👍

@samsonasik samsonasik merged commit 50378fc into rectorphp:main Apr 27, 2023
38 checks passed
@plenka
Copy link

plenka commented Apr 28, 2023

@mickeytodd @samsonasik Thanks for fixing this so quickly!

samsonasik pushed a commit that referenced this pull request May 8, 2023
…WithDefaultValueToConstantRector (#3687)

* fix: checking for other modifying types

* fix: added fixture

* fix: changed class name

* fix: checking if parent class is null

* fix: checking for all types of assigning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants