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

Add Method addTips() to Rule to #218

Merged
merged 11 commits into from
Aug 12, 2023
Merged

Conversation

verfriemelt-dot-org
Copy link
Contributor

@verfriemelt-dot-org verfriemelt-dot-org commented Jun 2, 2023

This allows to add reasoning about any given rule via the addTips() implementation of PHPStan.

consider the following rule

    public function test_no_concrete_repository(): Rule
    {
        return PHPat::rule()
            ->classes(Selector::namespace('\\Foo\\Bar'))
            ->shouldNotDependOn()
            ->classes(
                Selector::namespace('\\Foo\\Bar\\Repository'),
            )
            ->because(
                'you should instead depend on an abstraction!',
                'its for your own good!'
            )
        ;
    }

which would yield

 ------ ------------------------------------------------------------------------------------------------
  Line   src/Domain/Entity/LineImportService.php
 ------ ------------------------------------------------------------------------------------------------
  12     Foo\Bar\Domain\Entity\LineImportService should not depend on \Foo\Bar\Repository
         💡 you should instead depend on an abstraction!
         💡 its for your own good!

 ------ ------------------------------------------------------------------------------------------------

this can add value via reasons or alternative approaches or generic tips.

Copy link
Owner

@carlosas carlosas left a comment

Choose a reason for hiding this comment

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

Requested some changes :)
Also missing a test (with no tips, with one tip, with more than one tip)

src/Rule/Assertion/Relation/ValidationTrait.php Outdated Show resolved Hide resolved
src/Test/Builder/AbstractStep.php Outdated Show resolved Hide resolved
@verfriemelt-dot-org
Copy link
Contributor Author

i tried to implement your suggestions and added an example in the README.md as well.
let me know if i can improve something :)

@carlosas
Copy link
Owner

carlosas commented Jul 9, 2023

It's working well for Relation Assertions (depend on, extend...), but it's missing for Declaration Assertions (is final, is abstract...)

In AssertionStep, those assertion methods should be like:

public function shouldBeFinal(): TipOrBuildStep
{
    $this->rule->assertion = ShouldBeFinal::class;

    return new TipOrBuildStep($this->rule);
}

and also src/Rule/Assertion/Declaration/ValidationTrait.php should build the error with the tips, like src/Rule/Assertion/Relation/ValidationTrait.php does :)


The CI checks give an error because the php-cs-fixer configuration includes a rule that breaks on PHP 7.4. I will update the config now. Please, rebase or merge from master and edit src/Test/TestExtractor.php (line 35) manually back to:

$reflectedTest = $this->reflectTest(get_class($test));

@verfriemelt-dot-org
Copy link
Contributor Author

and also src/Rule/Assertion/Declaration/ValidationTrait.php should build the error with the tips, like src/Rule/Assertion/Relation/ValidationTrait.php does

that makes sense :) i'll fix that

@verfriemelt-dot-org
Copy link
Contributor Author

i thought about using a generator and basically test every method for the ValidationTraits ... but that would be more or less a full duplicate of all the other testcases and not really diserable 🤔 what do you think?

@carlosas
Copy link
Owner

i thought about using a generator and basically test every method for the ValidationTraits ... but that would be more or less a full duplicate of all the other testcases and not really diserable 🤔 what do you think?

the current tests are good enough :) merging and will deploy soon together with a couple of new features
sorry for the delay! 🌴

@carlosas carlosas merged commit e1ce34e into carlosas:master Aug 12, 2023
@verfriemelt-dot-org
Copy link
Contributor Author

yay 🎉 thanks for merging that :)

@carlosas
Copy link
Owner

thanks for the contribution 🙂

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.

2 participants