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

Do not run ChangeReadOnlyPropertyWithDefaultValueToConstantRector on properties with attributes #1694

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 18, 2022

No description provided.

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

Seems like I'm responsible for tests failure, investigating

@TomasVotruba
Copy link
Member

It rather looks like bug between Symfony and Paratest with color string: https://github.com/rectorphp/rector-src/runs/4850475081?check_suite_focus=true#step:5:88

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

Yup, I noticed that but master is green 🤔

I'm waiting for test suite to finish on my local machine.

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

There are ~3 test failures that are hidden in the output by the paratest bug you referenced.

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 18, 2022

Yup, I noticed that but master is green thinking

That usually means new version (of paratest) was release since last PR was merged.

@TomasVotruba
Copy link
Member

You can lock the paratest to last lower version in this PR to see if it affects result on CI.

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

Last release was in December :D never mind, I'll try to fix that one too.

@TomasVotruba
Copy link
Member

That can be allowed by another dependency in another package. With composer you never know what you get :)

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

True true

@TomasVotruba
Copy link
Member

Hm, the last passing PR (#1669) had:

Locking brianium/paratest (v6.4.1)

This one has the same:

Installing brianium/paratest (v6.4.1): Extracting archive

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

IMO the bug is there for a while already but I've just found it.

I'm getting closer

image

@TomasVotruba
Copy link
Member

I see...

When I check out this PR and run:

pu rules-tests/Privatization/Rector/Property/ChangeReadOnlyPropertyWithDefaultValueToConstantRector/ChangeReadOnlyPropertyWithDefaultValueToConstantRectorTest.php

The test fails.

The paratest has some bug to interpret these changes correctly, that we discovered by accident.

@TomasVotruba
Copy link
Member

Without your change, only the added fixture fails.

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

Yup

@TomasVotruba
Copy link
Member

I think I know :)

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

My condition is wrong

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 18, 2022

It's the naming issue. In 2011, when the php-parser was born, nobody knew the native annotations will come to PHP and will be called attributes.So Nikita used "attributes" name for the metadata informations.

Now we have native #[attributes], so to differentiate, they are called attrGroup in php-parser node.

I'd prefer to name first to "metadata" and second to "attributes", but in 2025 we might have native "metadata " feature in PHP and we're in the same position :)

#1695

@TomasVotruba
Copy link
Member

Feel free to update this PR, I can merge it then. Thanks 👍

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

Cool. Was not familiar with the API so thanks for pointer.

@TomasVotruba
Copy link
Member

Thanks for the improvement 👏

I got burned this ~5 times already 😆

@simPod
Copy link
Contributor Author

simPod commented Jan 18, 2022

I tried to fix the paratest as well paratestphp/paratest#650

@TomasVotruba
Copy link
Member

@simPod That's amazing, thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants