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 no class feature #316

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/CLI/TargetPhpVersion.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ private function __construct(?string $version)
$this->version = $version;
}

public static function create(?string $version): self
/**
* @param ?string $version
*
* @throws PhpVersionNotValidException
*/
public static function create(?string $version = null): self
{
if (null === $version) {
return new self(phpversion());
Expand Down
57 changes: 57 additions & 0 deletions src/Expression/NegateDecorator.php
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
Copy link
Contributor

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 expressions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree!

{
/** @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))
)
);
}
}
}
42 changes: 42 additions & 0 deletions src/Rules/NoClass.php
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;
}
}
5 changes: 5 additions & 0 deletions src/Rules/Rule.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,9 @@ public static function allClasses(): AllClasses
{
return new AllClasses();
}

public static function noClass(): NoClass
Copy link
Contributor

Choose a reason for hiding this comment

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

what about naming it NoClasses, for consistency with AllClasses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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".

Copy link
Contributor

Choose a reason for hiding this comment

The 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();
}
}
14 changes: 12 additions & 2 deletions src/Rules/RuleBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
namespace Arkitect\Rules;

use Arkitect\Expression\Expression;
use Arkitect\Expression\NegateDecorator;

class RuleBuilder
{
Expand All @@ -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
Expand All @@ -39,6 +45,10 @@ public function addThat(Expression $that): self

public function addShould(Expression $should): self
{
if ($this->negateShoulds) {
$should = new NegateDecorator($should);
}
Copy link
Contributor

@micheleorselli micheleorselli Nov 23, 2022

Choose a reason for hiding this comment

The 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?
If the should method in NoClass already does this

$this->ruleBuilder->addShould(new NegateDecorator($expression));

why do we need to add another new NegateDecorator here?


$this->shoulds->add($should);

return $this;
Expand Down Expand Up @@ -69,9 +79,9 @@ public function classesToBeExcluded(string ...$classesToBeExcluded): self
return $this;
}

public function setRunOnlyThis(): self
public function negateShoulds(): self

Choose a reason for hiding this comment

The 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;
}
Expand Down
3 changes: 1 addition & 2 deletions src/Rules/Specs.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

class Specs
{
/** @var array */
/** @var list<Expression> */
private $expressions = [];

public function add(Expression $expression): void
Expand All @@ -18,7 +18,6 @@ public function add(Expression $expression): void

public function allSpecsAreMatchedBy(ClassDescription $classDescription, string $because): bool
{
/** @var Expression $spec */
foreach ($this->expressions as $spec) {
$violations = new Violations();
$spec->evaluate($classDescription, $violations, $because);
Expand Down
56 changes: 56 additions & 0 deletions tests/Expression/NegateDecoratorTest.php
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());
}
}
64 changes: 64 additions & 0 deletions tests/Unit/Rules/NoClassRulesTest.php
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());
}
}