phpat icon indicating copy to clipboard operation
phpat copied to clipboard

Dynamic selectors

Open ramonacat opened this issue 4 years ago • 4 comments

Enhancement description My code is structured in such a way that the top level namespaces do not depend on each other, except for some strictly defined exceptions, e.g. assuming I have the following classes/interfaces:

  • App\Weather\Infrastructure\WeatherController
  • Graphs\Infrastructure\Renderer
  • Graphs\Api\RendererInterface

I would like to define rules such that WeatherController cannot depend on Renderer, but can depend on RendererInterface. This can be currently done like this:

return $this
            ->newRule
            ->classesThat(Selector::haveClassName('App\\Weather\\*'))
            ->canOnlyDependOn()
            ->classesThat(Selector::haveClassName('App\\Weather\\*'))
            ->andClassesThat(Selector::haveClassName('App\\*\\Api\\*'))
            ->build();

The issue with that approach is that it requires repetition for each top-level namespace.

Suggested approach or solution My dream API would be something like this, altough I'm not sure if that's really the best approach:

return $this
            ->newRule
            ->classesThat(Selector::haveClassName('App\\{level1}\\*'))
            ->canOnlyDependOn()
            ->classesThat(Selector::haveClassName('App\\{level1}\\*'))
            ->andClassesThat(Selector::haveClassName('App\\*\\Api\\*'))
            ->build();

ramonacat avatar Jul 17 '20 17:07 ramonacat

This is actually a great idea 😮 but it might be a bit tricky to implement. If you (or someone else) want to contribute with this feature it will be more than welcome. I will explain a bit how it works.

There is something called StatementBuilder which transforms Rules into Statements by resolving the selectors and splitting the pack of classes that must be tested. A Rule is composed by Selector(s) + Assertion + Selector(s) and a Statement is composed by ClassName + Assertion + ClassName(s)/RegexClassName(s). Being in both cases named 'origin' + assertion + 'destinations'.

Something like 'App\\{level1}\\Domain\\*' should be sent to resolve like 'App\\*\\Domain\\*' (probably forbidding \ when that first * gets transformed to real regex) and then keep track of which part of the result classnames corresponds to the variable, so the destination classnames can be filtered to match that pattern. This filled variable could be added to the destination selector so it gets replaced before resolving it.

Note: when resolving the selectors in the StatementBuilder, be sure getSrcIncluded and getSrcExcluded options still apply.

I'm focused on some other improvements right now so I will tag the enhancement with help wanted, but I can help anyone willing to contribute.

carlosas avatar Jul 18 '20 09:07 carlosas

So, I attempted implementing it, but there seems to be an issue - since StatementBuilder evaluates all the origins and all the destination before building the statement, and we need a many to many mapping between the origins and destinations, that information is lost by the time the assertion happens (it can't be simply substituted in the destination selector, because there are multiple possible values, depending on the destination namespace). I'm wondering if maybe a solution that'd allow dynamically creating rules would be better instead, something like:

/** @return Rule[] */
public function testTopLevelNamespaces(): array
{
    $rules = [];
    foreach(NamespaceEnumerator::find('App\\*') as $namespace) {
        $rules = $this->newRule
             ->classesThat(Selector::haveName($namespace . '\\*'))
             ->canOnlyDependOn()
             ->classesThat(Selector::haveName($namespace . '\\*'))
    }
    return $rules;
}

I think implementing that would be way easier, while roughly providing the same possibilities. What do you think?

ramonacat avatar Jul 18 '20 18:07 ramonacat

Sorry for the late response. Busy days.

I would accept having an exposed version of the SelectorResolver, but that's not exactly your need. You will need a method that returns the first portion of the namespace for a given regex. That looks more like an ad-hoc feature, and IMO would also (kind of) dirty the DSL spirit. Of course it would be easier to implement but reduces a lot the versatility. For instance, someone may want to select App\\{var1}\\Something\\{var2}\\*. Also making the methods to return an array of rules instead of a single Rule, is a BC break which is not desirable 😅 I think its worth the effort to modify the StatementBuilder to be able to replace this variables at the right moment.

Maybe this could be done with a new ClassLike implementation (maybe VariableClassName) that can transform itself to Regex (like I mentioned), saving the position of the variables to be resolved later with something like

public function resolve(array $replacements): ClassLike
{
//TODO: implement
}

StatementBuilder::selectOrigins could give the variable values found and inside the foreach origin use those values to transform each of the destination VariableClassName into FullClassName or RegexClassName.

This approach is just a brief idea but I think it might be done without excesive complications.

carlosas avatar Jul 22 '20 17:07 carlosas

No worries, I'm also rather bust these days.

The approach you're proposing sounds reasonable, I'll try it Soon™

ramonacat avatar Jul 28 '20 15:07 ramonacat