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

Mutate public to private methods #500

Closed
wants to merge 1 commit into from

Conversation

deleugpn
Copy link

@deleugpn deleugpn commented Oct 2, 2018

This PR:

Fixes #498

This PR adds the ability to mutate from public straight to private methods.

@sanmai
Copy link
Member

sanmai commented Oct 2, 2018

  1. CI is failing because you need to run E2E tests and update fixtures for them (as now they see more mutations). The latter is typically done with ./tests/e2e_tests.
  2. I'd make a private function makeClassMethod($visiblity): ClassMethod and avoid copy'n'pasting a bunch of code, what do you think?

@deleugpn
Copy link
Author

deleugpn commented Oct 3, 2018

Sorry, I did this PR at GitHub opening event in Amsterdam and forgot to check back the status. I'll look into the e2e test.
I swear I tried making a private method as you said and it was breaking the mutation.
I'll try again =D

@sanmai
Copy link
Member

sanmai commented Oct 4, 2018

Please do. I'm here if you need a hand.

@sanmai sanmai added the Mutator label Oct 4, 2018
*/
public function mutate(Node $node)
{
/* @var ClassMethod $node */
return new ClassMethod(
yield new ClassMethod(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What improvement adds the yield here instead of the return?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a new feature (not yet released). It allows to return N mutations from one mutator. Note the second mutation on line 48 below.

Also, this is not the same as return [new ClassMethod(...), new ClassMethod(...)]. Such code replaces 1 Node with an array of Nodes, which is completely different behaviour.

@maks-rafalko
Copy link
Member

Are you going to finish this PR @deleugpn or can we take over it?

@deleugpn
Copy link
Author

deleugpn commented Apr 9, 2019

Sorry, you can take it over. I hope to be back at some point.

@deleugpn deleugpn closed this Apr 9, 2019
@ghost ghost removed the Needs Review label Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants