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

Run method call prohibition analyzer when cloning #8155

Merged
merged 1 commit into from Jun 24, 2022

Conversation

danog
Copy link
Collaborator

@danog danog commented Jun 24, 2022

May or may not be used later in the immutable refactoring.

@AndrolGenhald
Copy link
Collaborator

Hmm, I'm not too sure what the best way to handle this is. Usually deprecated methods are intended to be removed in the future, but if a deprecated __clone is removed that would mean Psalm would start to allow cloning again with no warning. I think a @no-clone tag on the class would be better, but I don't know how useful it would actually be in practice.

@orklah
Copy link
Collaborator

orklah commented Jun 24, 2022

Well if we want to deprecate cloning, we tag it as deprecated but instead of removing the method later, we will remove the deprecated tag, make the function private and throw an exception in it. That way the method won't be called again

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Jun 24, 2022

I was wondering if adding something like @no-clone would be better so Psalm could check it instead of just failing at runtime, but maybe it's not a common enough use-case to be worth adding.

Edit: nvm, I'm an idiot, this already works. LGTM then!

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jun 24, 2022
@orklah orklah merged commit 8b7bc07 into vimeo:master Jun 24, 2022
@orklah
Copy link
Collaborator

orklah commented Jun 24, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants