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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check for class const with reserved word 'class' #8542

Merged
merged 1 commit into from Oct 7, 2022

Conversation

gphargreaves
Copy link
Contributor

First contribution so hopefully my changes are in sensible places 馃榿 Addresses issue #8294

Had a read through the docs for the a structure of the analyzers and hopefully this change is fit for purpose, happy to tweak accordingly if you have any feedback.

@orklah
Copy link
Collaborator

orklah commented Oct 7, 2022

Looks good!

Note that you targeted master, which is the branch for Psalm 5 which is scheduled for release in a few weeks if all works out.

If you want to be sure to have this change on Psalm 4, you could target the 4.x branch, your choice :)

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Oct 7, 2022
@gphargreaves gphargreaves changed the base branch from master to 4.x October 7, 2022 08:47
@AndrolGenhald
Copy link
Collaborator

LGTM, not sure what's up with CI though. I think I'd prefer this go in ClassConstAnalyzer, but since it targets 4.x this spot is fine (that whole loop should probably be moved to ClassConstAnalyzer in 5.x just to keep things better organized).

@orklah orklah merged commit 52e96be into vimeo:4.x Oct 7, 2022
@orklah
Copy link
Collaborator

orklah commented Oct 7, 2022

Thanks!

CI often go nuts when retargeting PRs, I think it reverts back to some old version of CI from somewhere

@gphargreaves gphargreaves deleted the class-constant-named-class branch October 7, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants