Skip to content

Conversation

@micheleorselli
Copy link
Member

This is a tentative fix for #434. It introduces a new method, appliesTo, which is used to decide if a given rules should be applied to the class under examination, eg:

  • final classes, enums, interfaces, trait, cannot be abstract so we exclude them when we check if a given class is abstract or not, because they cannot have that property by definition.

Caveat: with this fix a rule defined in that way

$rule = Rule::allClasses()
            ->that(new ResideInOneOfTheseNamespaces('App\Abstract'))
            ->should(new IsAbstract())
            ->because('we want to prefix abstract classes');

would miss any final class present in the namesapce App\Abstract. Is that what we want?

@codecov
Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.79%. Comparing base (13d5528) to head (7e3923a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/PHPUnit/ArchRuleCheckerConstraintAdapter.php 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #454      +/-   ##
============================================
+ Coverage     93.62%   94.79%   +1.16%     
- Complexity      593      606      +13     
============================================
  Files            69       69              
  Lines          1601     1614      +13     
============================================
+ Hits           1499     1530      +31     
+ Misses          102       84      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fain182
Copy link
Collaborator

fain182 commented Feb 27, 2025

would miss any final class present in the namespace App\Abstract. Is that what we want?

I don't think this would be the expected behaviour from our users....

@micheleorselli micheleorselli marked this pull request as ready for review March 3, 2025 22:19
$this->classDescriptionBuilder
->addDependency(new ClassDependency(implode('\\', $nodeName->getParts()), $node->getLine()));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this snippet was adding the dependency derived by attributes twice


/**
* @requires PHP >= 8.0
*/
Copy link
Member Author

Choose a reason for hiding this comment

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

removed because even if we are on php 7.4 php parser can parse php 8 code.


public function appliesTo(ClassDescription $theClass): bool
{
return !($theClass->isInterface() || $theClass->isTrait() || $theClass->isEnum() || $theClass->isFinal());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isFinal should not be there?

@micheleorselli micheleorselli merged commit 92dad93 into main Mar 7, 2025
34 checks passed
@micheleorselli micheleorselli deleted the split-that-and-should-check-in-rules branch March 7, 2025 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants