Skip to content

Implement no class feature#316

Draft
pfazzi wants to merge 2 commits intomainfrom
no-class
Draft

Implement no class feature#316
pfazzi wants to merge 2 commits intomainfrom
no-class

Conversation

@pfazzi
Copy link
Collaborator

@pfazzi pfazzi commented Nov 22, 2022 •
edited
Loading

Closes #315 allowing to write rules like:

Rule::noClass()
  ->should(new ResideInTheseNamespaces('App\Services'))
  ->because('this namespace has been deprecated in favor of the modular architecture');

or

Rule::noClass()
  ->that(new ResideInOneOfTheseNamespaces('App\Entity'))
  ->should(new HaveNameMatching('*Service'))
  ->because('of our naming convention');

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022 •
edited
Loading

Codecov Report

Merging #316 (cd1fda2) into main (392757f) will increase coverage by 0.20%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##               main     #316      +/-   ##
============================================
+ Coverage     93.02%   93.23%   +0.20%     
- Complexity      476      487      +11     
============================================
  Files            60       62       +2     
  Lines          1262     1301      +39     
============================================
+ Hits           1174     1213      +39     
  Misses           88       88
Impacted Files Coverage Δ
src/Rules/Specs.php 100.00% <ø> (ø)
src/Rules/NoClass.php 72.72% <72.72%> (ø)
src/CLI/TargetPhpVersion.php 100.00% <100.00%> (ø)
src/Expression/NegateDecorator.php 100.00% <100.00%> (ø)
src/Rules/Rule.php 100.00% <100.00%> (ø)
src/Rules/RuleBuilder.php 100.00% <100.00%> (+10.71%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pfazzi pfazzi marked this pull request as draft November 22, 2022 22:48
AlessandroMinoccheri
AlessandroMinoccheri reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@AlessandroMinoccheri AlessandroMinoccheri left a comment

Choose a reason for hiding this comment

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

I would suggest to add a description of this new behavior into the README file

src/Rules/RuleBuilder.php
}

public function setRunOnlyThis(): self
public function negateShoulds(): self
Copy link
Member

@AlessandroMinoccheri AlessandroMinoccheri Nov 23, 2022

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?

micheleorselli
micheleorselli reviewed Nov 23, 2022
View reviewed changes
src/Expression/NegateDecorator.php
use Arkitect\Rules\ViolationMessage;
use Arkitect\Rules\Violations;

class NegateDecorator implements Expression
Copy link
Member

@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.

What about naming it just Not? I could also simplify other expressions

Copy link
Collaborator Author

@pfazzi pfazzi Nov 23, 2022

Choose a reason for hiding this comment

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

Totally agree!

src/Rules/Rule.php
return new AllClasses();
}

public static function noClass(): NoClass
Copy link
Member

@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.

what about naming it NoClasses, for consistency with AllClasses?

Copy link
Collaborator Author

@pfazzi pfazzi Nov 23, 2022

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
Member

@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.

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);

src/Rules/RuleBuilder.php
{
if ($this->negateShoulds) {
$should = new NegateDecorator($should);
}
Copy link
Member

@micheleorselli micheleorselli Nov 23, 2022 •
edited
Loading

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?

@fain182 fain182 added this to the v1 milestone Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@micheleorselli micheleorselli micheleorselli left review comments

@AlessandroMinoccheri AlessandroMinoccheri AlessandroMinoccheri left review comments

@fain182 fain182 Awaiting requested review from fain182

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1

Development

Successfully merging this pull request may close these issues.

No classes should DSL

5 participants

@pfazzi @codecov-commenter @micheleorselli @AlessandroMinoccheri @fain182