implement except(...) clause to improve expressivity

[ricfio]

Feature Request

Q	A
New Feature	yes
RFC	yes
BC Break	no

Summary

We may need a new exception clause (not a php exception) to apply to Rule::allClasses() that at this time does not exists (I believe).

As earlier discussed (@micheleorselli)...

Is something we discussed earlier and, at that time, we decided to not include it in the DSL. Main reason: ->except could be interpreted as a way to

• exclude classes from being evaluated (allClasses -> except)
• exclude classes from the that clause (that(ResideInOneOfTheseNamespaces) -> except)
• exclude classes from the should clause (should(NotHaveDependenciesOutside) -> except)

[AlessandroMinoccheri]

What do you think about something like this?

$rule = Rule::allClasses()
            ->that(new ResideInOneOfTheseNamespaces('App\HappyIsland'))
            ->should(new HaveNameMatching('Happy*'), new RuleException('App\HappyIsland\Foo'))
            ->because("that's what she said");

We can add to should method a nullable parameter for exceptions (also for that method?)

What do you think @micheleorselli @fain182 ?

At the moment I am working on something like this solution but there Is many code to edit but it seems to works fine.

[fain182]

I think that the selection of the classes should be in the "that" clause...
so I would think something like:

$rule = Rule::allClasses()
            ->that(new ResideInOneOfTheseNamespaces('App\HappyIsland'))
            ->andThat(new IsNotOneOf([''App\HappyIsland\Foo'']))
            ->should(new HaveNameMatching('Happy*'))
            ->because("that's what she said");

I am not sure about the name IsNotOneOf, even if reading it in the that clause sounds good...

[AlessandroMinoccheri]

For the selection of the class should be in that I agree, but if you want to exclude some dependencies from the rules you need to use it in should.

isNotOneOf is a better name, thanks @fain182

[AlessandroMinoccheri]

@fain182 but what do you think about adding a second parameter to should?
Like this:

->should(new HaveNameMatching('Happy*'), new isNotOneOf('App\HappyIsland\Foo'))

[fain182]

It's not too bad to read, but how would you call in the signature the second argument of the should ?
I am trying to see if this solution can be generalized or not, because if we add a second argument to the "should", it should make sense always...

[AlessandroMinoccheri]

In my mind, @fain182 , should function could be something like this:

public function should(Expression $expression, ? isNotOneOf $isNotOneOf): BecauseParser

We can use a nullable argument, I know it's not the best solution but adding another layer like:

->should(new HaveNameMatching('Happy*'))
->andShould(new isNotOneOf('HappyFoo*'))

This solution could be more complicated at the moment.

[ricfio]

In my opinion will be great have a method such as Except instead of to pass the expression exceptions in a parameter.

I don't know if it is applicable to the current codebase, if yes, I prefer have different methods with the same name Except but different behaviour according to the context where it is called... for example:

->that(new ResideInOneOfTheseNamespaces('App\HappyIsland')->Except(...))

->should(new HaveNameMatching('Happy*')->Except(...))

[AlessandroMinoccheri]

We have added a new feature in this merged PR: #219 @ricfio

[AlessandroMinoccheri]

We have added a new feature to exclude some dependencies here: #220

Can we consider now this issue closed @ricfio ?

[ricfio]

Well, with the new feature added in the #220 I have solved all my issues updating my rule as follow:

    $rules[] = Rule::allClasses()
        ->that(new ResideInOneOfTheseNamespaces('App\Domain'))
        ->should(new NotHaveDependencyOutsideNamespace('App\Domain', [
            'DateTimeImmutable',
            //
            'ArrayIterator',
            'Countable', 
            'Collection',
            'IteratorAggregate',
            'JsonSerializable',
            'Stringable',
            //
            'InvalidArgumentException',
            'RangeException',
            'RuntimeException',
            //
            'ReflectionClass',
            //
            'Doctrine\Common\Collections\ArrayCollection',
            'Doctrine\Common\Collections\Collection',
            'Ramsey\Uuid\Uuid',
            'Webmozart\Assert\Assert',
        ]))
        ->because('we want protect our domain');

So for me this issue can be closed.

PS
However I think it's not very convenient to have to list all those namespaces in my rule.
Have you any alternative ideas to solve in more elegant way?

[AlessandroMinoccheri]

You can use a variable as an array to contain all those classes

[ricfio]

Sure, I could also grouping the namespaces in less rows, putting more in a row, but the question is not this.

The real question is:
Has it sense to consider the root namespace \ and then the root classes (such as \DateTimeImmutable, \Stringable, and so on) as external dependency?
In my opinion no because the root classes are part of the standard classes available in PHP.

So why do not always consider this class to be not external to your namespace?
I think that this would be a more convenient way to use phparkitect dependencies rules.

PS.
Could I write the same above rule in this way?

$rules[] = Rule::allClasses()
        ->that(new ResideInOneOfTheseNamespaces('App\Domain'))
        ->should(new NotHaveDependencyOutsideNamespace('App\Domain', [
            '\',
            'Doctrine\Common\Collections\ArrayCollection',
            'Doctrine\Common\Collections\Collection',
            'Ramsey\Uuid\Uuid',
            'Webmozart\Assert\Assert',
        ]))
        ->because('we want protect our domain');

.. replacing all root classes with '' or eventually with '*' (wildchars) ?

[fain182]

Maybe we could avoid always to check dependency on PHP built-in classes, using something like this: https://www.php.net/manual/en/reflectionclass.isinternal.php...