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

Implement UnwrapArrayMap and UnwrapArrayFilter mutators #513

Merged
merged 3 commits into from Oct 19, 2018

Conversation

akondas
Copy link
Contributor

@akondas akondas commented Oct 17, 2018

This PR:

Inspiration in #490

Fixes #490

return $node->args[$this->getParameterIndex()];
}

protected function mutatesNode(Node $node): bool
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be final, what do you think?

Suggested change
protected function mutatesNode(Node $node): bool
final protected function mutatesNode(Node $node): bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definietly :)

*
* @return Node\Param;
*/
public function mutate(Node $node)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function mutate(Node $node)
final public function mutate(Node $node)

sanmai
sanmai previously approved these changes Oct 18, 2018
Copy link
Member

@sanmai sanmai left a comment

Choose a reason for hiding this comment

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

Outside of some minor comments, looks good to me.

<<<'PHP'
<?php

$a = array_filter(['A', 1, 'C'], function($var): bool {
Copy link
Member

Choose a reason for hiding this comment

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

could you please add more tests (like parameter as a variable, function with lower/upper cased characters and so on.)

See the list of cases in https://github.com/infection/infection/blob/68f70ad62ffd14af62781d989e4230c45c235010/tests/Mutator/Regex/PregQuoteTest.php

same for array_map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, wrongly capitalized was the problem :)

Copy link
Member

Choose a reason for hiding this comment

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

There's $node->name->toLowerString() of use.

sanmai
sanmai previously approved these changes Oct 18, 2018
PHP
];

yield 'It mutates correctly when array_map is wrongly capitalized' => [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield 'It mutates correctly when array_map is wrongly capitalized' => [
yield 'It mutates correctly when array_filter is wrongly capitalized' => [

PHP
];

yield 'It mutates correctly when array_map uses another function as input' => [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
yield 'It mutates correctly when array_map uses another function as input' => [
yield 'It mutates correctly when array_filter uses another function as input' => [

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Cool 👍

@akondas
Copy link
Contributor Author

akondas commented Oct 19, 2018

Can we restart CI jobs? Failed loading C:\tools\php\ext\xdebug.dll it looks like an unreliable error.

@maks-rafalko maks-rafalko added this to the 0.11.0 milestone Oct 19, 2018
@maks-rafalko
Copy link
Member

Thank you @akondas, nice work.

The build is green now

@maks-rafalko
Copy link
Member

@akondas can I ask you to update documentation, please? See infection/site#70

@maks-rafalko
Copy link
Member

I just realized that with this new base abstract class, we can implement not only array functions mutators as mentioned here, but for any other function, for example

- return strtolower($string);
+ return $string;

👍

@sanmai
Copy link
Member

sanmai commented Oct 19, 2018

Yeah, and things like:

- return array_reduce($array, $func, $initial);
+ return $initial;

Should we make a list within an issue with ideas?

@maks-rafalko maks-rafalko mentioned this pull request Oct 19, 2018
46 tasks
@maks-rafalko
Copy link
Member

good idea, done #514

@localheinz localheinz mentioned this pull request Oct 29, 2018
1 task
@@ -159,6 +160,11 @@
Mutator\Cast\CastString::class,
];

public const UNWRAP = [
Mutator\Unwrap\UnwrapArrayMap::class,
Mutator\Unwrap\UnwrapArrayFilter::class,
Copy link
Member

Choose a reason for hiding this comment

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

@akondas @borNfreee @sanmai

Is the order important here?

If not, maybe we can sort alphabetically?

Copy link
Member

Choose a reason for hiding this comment

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

Same order as in the manual should work best, IMO.


// Unwrap
'UnwrapArrayMap' => Mutator\Unwrap\UnwrapArrayMap::class,
'UnwrapArrayFilter' => Mutator\Unwrap\UnwrapArrayFilter::class,
Copy link
Member

Choose a reason for hiding this comment

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

@akondas @borNfreee @sanmai

Is the order important here?

If not, maybe we can sort alphabetically?

raphaelstolt pushed a commit to raphaelstolt/infection that referenced this pull request Nov 2, 2018
* Implement UnwrapArrayMap and UnwrapArrayFilter mutators

* Add more tests examples and fix wrongly capitalized funcation name

* Fix typos in tests provider for UnwrapArrayFilter mutator
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