Infection: Try to simplify Public API of Mutator class

[maks-rafalko]

This is how Mutator's public API looks like at the moment:

abstract class Mutator
{
    abstract public function mutate(Node $node);

    abstract public function shouldMutate(Node $node): bool;

    public static function getName(): string {}

    public function isIgnored(string $class, string $method): bool { }
}

Note the isIgnored method we've added recently.

If you look at the MutationsCollectorVisitor::leaveNode method logic, you will see that Mutator should not mutate the code if Mutator is ignored, right?

What I want to say is ->isIgnored() method should be an implementation detail of ->shouldMutate() method. So I suggest the following change:

public function shouldMutate(Node $node)
{
    if (!$this->someAbstractShouldMutate()) {
        return false;
    }

    return !$this->isIgnored($node);
}

private isIgnored() {} // it's private now (!)

Why does it bother me?

• Because it looks logical that we should not mutate when mutator is ignored

• One day, we will expose Mutator interface to the external world in order to allow users to implements their own mutators in the project that Infection will pick up, so we will promise some level of stability of this interface and every public method will be a pain for us.

[theofidry]

👍

[BackEndTea]

On that note, we should also make the getName() function final.

[sanmai]

In regard of users from the outside world I think we better leave shouldMutate() as it is. Whatever reasons there are Infection thinks a mutator should be ignored is its own internal business. Therefore, isIgnored should stay separate from shouldMutate().

Similar is also true about isIgnored(). If an outside user want to ignore certain mutators for whatever principle they follow, it's their own business. We don't get to limit them anyhow, we can't hide isIgnored() because a user should be able to override it. Therefore, isIgnored() can't be private.

[maks-rafalko]

Disagree. Currently, we have not the best design of our classes (Mutator at least) that brings us problems I mentioned in the description.

isIgnored() should not be public inside Mutator class, it's already public inside MutatorConfig.

If an outside user want to ignore certain mutators for whatever principle they follow, it's their own business.

If they want to extend/override isIgnored() logic, they should do it for MutatorConfig file, not for Mutator. You can think about it like different ignoring strategies. We (infection) provide our own. If it does not suite user's needs, they can provide another one and inject this strategy to Mutator class as a new MutatorConfig object. But this is for a far future though