-
-
Notifications
You must be signed in to change notification settings - Fork 41
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 no class feature #316
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
<?php | ||
declare(strict_types=1); | ||
|
||
namespace Arkitect\Expression; | ||
|
||
use Arkitect\Analyzer\ClassDescription; | ||
use Arkitect\Rules\Violation; | ||
use Arkitect\Rules\ViolationMessage; | ||
use Arkitect\Rules\Violations; | ||
|
||
class NegateDecorator implements Expression | ||
{ | ||
/** @var Expression */ | ||
private $expression; | ||
|
||
public function __construct(Expression $expression) | ||
{ | ||
$this->expression = $expression; | ||
} | ||
|
||
public function describe(ClassDescription $theClass, string $because): Description | ||
{ | ||
$description = $this->expression->describe($theClass, $because)->toString(); | ||
|
||
$description = str_replace( | ||
['should not'], | ||
['should'], | ||
$description, | ||
$count | ||
); | ||
|
||
if (0 === $count) { | ||
$description = str_replace( | ||
['should'], | ||
['should not'], | ||
$description, | ||
$count | ||
); | ||
} | ||
|
||
return new Description($description, ''); | ||
} | ||
|
||
public function evaluate(ClassDescription $theClass, Violations $violations, string $because): void | ||
{ | ||
$this->expression->evaluate($theClass, $currentViolations = new Violations(), $because); | ||
|
||
if (0 === $currentViolations->count()) { | ||
$violations->add( | ||
Violation::create( | ||
$theClass->getFQCN(), | ||
ViolationMessage::selfExplanatory($this->describe($theClass, $because)) | ||
) | ||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
<?php | ||
declare(strict_types=1); | ||
|
||
namespace Arkitect\Rules; | ||
|
||
use Arkitect\Expression\Expression; | ||
use Arkitect\Expression\NegateDecorator; | ||
use Arkitect\Rules\DSL\AndThatShouldParser; | ||
use Arkitect\Rules\DSL\BecauseParser; | ||
use Arkitect\Rules\DSL\ThatParser; | ||
|
||
class NoClass implements ThatParser | ||
{ | ||
/** @var RuleBuilder */ | ||
protected $ruleBuilder; | ||
|
||
public function __construct() | ||
{ | ||
$this->ruleBuilder = (new RuleBuilder())->negateShoulds(); | ||
} | ||
|
||
public function should(Expression $expression): BecauseParser | ||
{ | ||
$this->ruleBuilder->addShould(new NegateDecorator($expression)); | ||
|
||
return new Because($this->ruleBuilder); | ||
} | ||
|
||
public function that(Expression $expression): AndThatShouldParser | ||
{ | ||
$this->ruleBuilder->addThat($expression); | ||
|
||
return new AndThatShould($this->ruleBuilder); | ||
} | ||
|
||
public function except(string ...$classesToBeExcluded): ThatParser | ||
{ | ||
$this->ruleBuilder->classesToBeExcluded(...$classesToBeExcluded); | ||
|
||
return $this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,4 +9,9 @@ public static function allClasses(): AllClasses | |
{ | ||
return new AllClasses(); | ||
} | ||
|
||
public static function noClass(): NoClass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about naming it NoClasses, for consistency with AllClasses? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it would not be correct English. It's the same in Italian "tutte le classi"/"nessuna classe". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am far from being an expert but I think also NoClasses is correct. Looking at java's ArchUnit they decided to used it, eg. ArchRule rule = ArchRuleDefinition.noClasses()
.that().resideInAPackage("..service..")
.should().accessClassesThat().resideInAPackage("..controller..");
rule.check(importedClasses); |
||
{ | ||
return new NoClass(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
namespace Arkitect\Rules; | ||
|
||
use Arkitect\Expression\Expression; | ||
use Arkitect\Expression\NegateDecorator; | ||
|
||
class RuleBuilder | ||
{ | ||
|
@@ -18,16 +19,21 @@ class RuleBuilder | |
|
||
/** @var array */ | ||
private $classesToBeExcluded; | ||
|
||
/** @var bool */ | ||
private $runOnlyThis; | ||
|
||
/** @var bool */ | ||
private $negateShoulds; | ||
|
||
public function __construct() | ||
{ | ||
$this->thats = new Specs(); | ||
$this->shoulds = new Constraints(); | ||
$this->because = ''; | ||
$this->classesToBeExcluded = []; | ||
$this->runOnlyThis = false; | ||
$this->negateShoulds = false; | ||
} | ||
|
||
public function addThat(Expression $that): self | ||
|
@@ -39,6 +45,10 @@ public function addThat(Expression $that): self | |
|
||
public function addShould(Expression $should): self | ||
{ | ||
if ($this->negateShoulds) { | ||
$should = new NegateDecorator($should); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I am too low on ☕ but I can't wrap my head around this, would you mind explaining it to me?
why do we need to add another |
||
|
||
$this->shoulds->add($should); | ||
|
||
return $this; | ||
|
@@ -69,9 +79,9 @@ public function classesToBeExcluded(string ...$classesToBeExcluded): self | |
return $this; | ||
} | ||
|
||
public function setRunOnlyThis(): self | ||
public function negateShoulds(): self | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you do this change? |
||
{ | ||
$this->runOnlyThis = true; | ||
$this->negateShoulds = true; | ||
|
||
return $this; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
declare(strict_types=1); | ||
|
||
namespace Arkitect\Tests\Expression; | ||
|
||
use Arkitect\Analyzer\ClassDescription; | ||
use Arkitect\Expression\ForClasses\IsFinal; | ||
use Arkitect\Expression\ForClasses\IsNotFinal; | ||
use Arkitect\Expression\NegateDecorator; | ||
use Arkitect\Rules\Violations; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class NegateDecoratorTest extends TestCase | ||
{ | ||
public function test_positive_decoration(): void | ||
{ | ||
$finalClass = ClassDescription::build('Tests\FinalClass') | ||
->setFinal(true) | ||
->get(); | ||
|
||
$isFinal = new IsFinal(); | ||
|
||
$isFinal->evaluate($finalClass, $violations = new Violations(), 'of some reason'); | ||
self::assertEquals('FinalClass should be final because of some reason', $isFinal->describe($finalClass, 'of some reason')->toString()); | ||
|
||
self::assertEquals(0, $violations->count()); | ||
|
||
$isNotFinal = new NegateDecorator($isFinal); | ||
|
||
$isNotFinal->evaluate($finalClass, $violations = new Violations(), 'of some reason'); | ||
self::assertEquals('FinalClass should not be final because of some reason', $isNotFinal->describe($finalClass, 'of some reason')->toString()); | ||
|
||
self::assertEquals(1, $violations->count()); | ||
} | ||
|
||
public function test_negative_decoration(): void | ||
{ | ||
$finalClass = ClassDescription::build('Tests\FinalClass') | ||
->setFinal(true) | ||
->get(); | ||
|
||
$isNotFinal = new IsNotFinal(); | ||
|
||
$isNotFinal->evaluate($finalClass, $violations = new Violations(), ''); | ||
|
||
self::assertEquals(1, $violations->count()); | ||
self::assertEquals('FinalClass should not be final because of some reason', $isNotFinal->describe($finalClass, 'of some reason')->toString()); | ||
|
||
$isFinal = new NegateDecorator($isNotFinal); | ||
|
||
$isFinal->evaluate($finalClass, $violations = new Violations(), ''); | ||
|
||
self::assertEquals(0, $violations->count()); | ||
self::assertEquals('FinalClass should be final because of some reason', $isFinal->describe($finalClass, 'of some reason')->toString()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<?php | ||
declare(strict_types=1); | ||
|
||
namespace Arkitect\Tests\Unit\Rules; | ||
|
||
use Arkitect\Analyzer\FileParserFactory; | ||
use Arkitect\ClassSet; | ||
use Arkitect\ClassSetRules; | ||
use Arkitect\CLI\Progress\VoidProgress; | ||
use Arkitect\CLI\Runner; | ||
use Arkitect\CLI\TargetPhpVersion; | ||
use Arkitect\Expression\ForClasses\HaveNameMatching; | ||
use Arkitect\Expression\ForClasses\NotResideInTheseNamespaces; | ||
use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces; | ||
use Arkitect\Rules\ParsingErrors; | ||
use Arkitect\Rules\Rule; | ||
use Arkitect\Rules\Violations; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class NoClassRulesTest extends TestCase | ||
{ | ||
public function test_no_class_without_that_clause_dsl_works(): void | ||
{ | ||
$rule = Rule::noClass() | ||
->should(new NotResideInTheseNamespaces('App\Services')) | ||
->because('this namespace has been deprecated in favor of the modular architecture'); | ||
|
||
$classSet = ClassSet::fromDir(__DIR__.'/../../E2E/_fixtures/mvc'); | ||
|
||
$runner = new Runner(); | ||
|
||
$runner->check( | ||
ClassSetRules::create($classSet, $rule), | ||
new VoidProgress(), | ||
FileParserFactory::createFileParser(TargetPhpVersion::create()), | ||
$violations = new Violations(), | ||
new ParsingErrors() | ||
); | ||
|
||
self::assertNotEmpty($violations->toArray()); | ||
} | ||
|
||
public function test_no_class_dsl_works(): void | ||
{ | ||
$rule = Rule::noClass() | ||
->that(new ResideInOneOfTheseNamespaces('App\Entity')) | ||
->should(new HaveNameMatching('*Service')) | ||
->because('of our naming convention'); | ||
|
||
$classSet = ClassSet::fromDir(__DIR__.'/../../E2E/_fixtures/mvc'); | ||
|
||
$runner = new Runner(); | ||
|
||
$runner->check( | ||
ClassSetRules::create($classSet, $rule), | ||
new VoidProgress(), | ||
FileParserFactory::createFileParser(TargetPhpVersion::create()), | ||
$violations = new Violations(), | ||
new ParsingErrors() | ||
); | ||
|
||
self::assertEmpty($violations->toArray()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about naming it just
Not
? I could also simplify other expressionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree!