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.

Sign up for GitHub

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

Jump to bottom

Implement no class feature #316

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Implement no class feature #316

wants to merge 2 commits into from

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
@@ -69,9 79,9 @@ public function classesToBeExcluded(string ...$classesToBeExcluded): self
return $this;
}

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

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
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!

src/Rules/Rule.php
@@ -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);

src/Rules/RuleBuilder.php
@@ -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 •
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