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

feat: add support for psalm 5 #133

Merged
merged 9 commits into from
Dec 1, 2022
Merged

feat: add support for psalm 5 #133

merged 9 commits into from
Dec 1, 2022

Conversation

nesl247
Copy link
Contributor

@nesl247 nesl247 commented Nov 30, 2022

This is only a change to composer.json to see if CI passes.

Closes #132

This is only a change to `composer.json` to see if CI passes.
@orklah
Copy link
Collaborator

orklah commented Nov 30, 2022

Seems like the main obstacle seems to be a BC break in Psalm 5: https://github.com/vimeo/psalm/blob/master/UPGRADING.md?plain=1#L24

We would need to guard against that and use the correct method for each version. Or we could drop support for V4 and only use the new method

@nesl247
Copy link
Contributor Author

nesl247 commented Nov 30, 2022

I spent quite some time looking at this, and can't figure out how to make this work. The functionality that is supposed to be the newer version does not do what we want from what I can tell because the only time it sets the Node is if something is changed on the node as it's being visited. I'm not really familiar with psalm's internal workings, so hopefully you'll have better luck. I do have a feeling that they may have entirely broken the functionality that is needed here.

@orklah
Copy link
Collaborator

orklah commented Nov 30, 2022

Hey @danog would you have some time for an advice here? I didn't entirely followed what we did with the getChildNodes. Do you have an example of what we can do to replace it?

@danog
Copy link

danog commented Nov 30, 2022

What is being done here with getChildNodes is precisely the reason why that method was a bad idea, in this case it's being used to simply access the type_params property indirectly

@weirdan
Copy link
Member

weirdan commented Nov 30, 2022

I've updated a bunch of tests for Psalm 5 and marked them accordingly. If Psalm 4 support is going to be dropped, it would be easy to grep for Psalm 4 substring to drop unused tests.

@weirdan weirdan marked this pull request as ready for review December 1, 2022 01:26
@orklah
Copy link
Collaborator

orklah commented Dec 1, 2022

Looks good, thanks everyone! If we can keep V4 compatibility without too much effort, let's do that for now!

@orklah orklah merged commit 1b670de into psalm:2.x Dec 1, 2022
@nesl247 nesl247 deleted the patch-1 branch December 1, 2022 17:45
@olegpro
Copy link

olegpro commented Dec 2, 2022

@orklah can you bump version? :)

Thanks!

@orklah
Copy link
Collaborator

orklah commented Dec 2, 2022

done :)

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

Successfully merging this pull request may close these issues.

Psalm 5 Support
5 participants