Skip to content

added parsing and checking functions docblocks #302

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

Merged
AlessandroMinoccheri merged 1 commit into from
Oct 27, 2022
Merged

added parsing and checking functions docblocks #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions src/Analyzer/ClassDescription.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ClassDescription
/** @var bool */
private $abstract;

/** @var string */
/** @var list<string> */
private $docBlock;

/** @var list<FullyQualifiedClassName> */
Expand All @@ -45,7 +45,7 @@ public function __construct(
?FullyQualifiedClassName $extends,
bool $final,
bool $abstract,
string $docBlock = '',
array $docBlock = [],
array $attributes = []
) {
$this->FQCN = $FQCN;
Expand Down Expand Up @@ -163,15 +163,17 @@ public function isAbstract(): bool
return $this->abstract;
}

public function getDocBlock(): string
public function getDocBlock(): array
{
return $this->docBlock;
}

public function containsDocBlock(string $search): bool
{
if (str_contains($this->docBlock, $search)) {
return true;
foreach ($this->docBlock as $docBlock) {
if (str_contains($docBlock, $search)) {
return true;
}
}

return false;
Expand Down
10 changes: 5 additions & 5 deletions src/Analyzer/ClassDescriptionBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ClassDescriptionBuilder
/** @var bool */
private $abstract;

/** @var string */
/** @var list<string> */
private $docBlock;

/** @var list<FullyQualifiedClassName> */
Expand All @@ -44,7 +44,7 @@ private function __construct(
array $interfaces,
bool $final,
bool $abstract,
string $docBlock = '',
array $docBlock = [],
array $attributes = []
) {
$this->FQCN = $FQCN;
Expand All @@ -66,7 +66,7 @@ public static function create(string $FQCN): self
[],
false,
false,
'',
[],
[]
);
}
Expand Down Expand Up @@ -132,9 +132,9 @@ public function setAbstract(bool $abstract): self
return $this;
}

public function setDocBlock(string $docBlock): self
public function addDocBlock(string $docBlock): self
{
$this->docBlock = $docBlock;
$this->docBlock[] = $docBlock;

return $this;
}
Expand Down
12 changes: 6 additions & 6 deletions src/Analyzer/FileVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ public function enterNode(Node $node): void
$this->classDescriptionBuilder->setAbstract(true);
}

if (null !== $node->getDocComment()) {
/** @var Doc $docComment */
$docComment = $node->getDocComment();
$this->classDescriptionBuilder->setDocBlock($docComment->getText());
}

foreach ($node->attrGroups as $attributeGroup) {
foreach ($attributeGroup->attrs as $attribute) {
$this->classDescriptionBuilder
Expand Down Expand Up @@ -133,6 +127,12 @@ public function enterNode(Node $node): void
);
}

if (null !== $this->classDescriptionBuilder && null !== $node->getDocComment()) {
Copy link
Member

@micheleorselli micheleorselli Oct 19, 2022

Choose a reason for hiding this comment

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

do we need the null check on $this->classDescriptionBuilder? I find it a bit confusing, as we are already using $this->classDescriptionBuilder a few lines above without any check

Copy link
Member Author

@AlessandroMinoccheri AlessandroMinoccheri Oct 19, 2022

Choose a reason for hiding this comment

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

I have got different errors without this check

Copy link
Member

@micheleorselli micheleorselli Oct 19, 2022

Choose a reason for hiding this comment

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

I am not sure I got what you mean but just out of curiosity: doesn't the check on line 28 prevent from having a null in $this->classDescriptionBuilder here?

And just to be super clear: all good from me, feel free to merge it

Copy link
Member Author

@AlessandroMinoccheri AlessandroMinoccheri Oct 21, 2022

Choose a reason for hiding this comment

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

The check on line 28 is inside an if that finish before the new check, this check is more generic

Copy link
Collaborator

@fain182 fain182 Oct 22, 2022

Choose a reason for hiding this comment

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

It's a little bit confusing, if tou want to split this method this could be a great refactoring... but if you prefer we can work on it on another PR.

Copy link
Member Author

@AlessandroMinoccheri AlessandroMinoccheri Oct 26, 2022

Choose a reason for hiding this comment

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

split the if @fain182 ? What is confusing in this part?

/** @var Doc $docComment */
$docComment = $node->getDocComment();
$this->classDescriptionBuilder->addDocBlock($docComment->getText());
}

/**
* matches parameters dependency in functions and method definitions like
* public function __construct(Symfony\Component\HttpFoundation\Request $request).
Expand Down
6 changes: 3 additions & 3 deletions tests/Unit/Analyzer/ClassDescriptionBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function test_it_should_create_annotated_class(): void
{
$FQCN = 'HappyIsland';
$classDescriptionBuilder = ClassDescriptionBuilder::create($FQCN);
$classDescriptionBuilder->setDocBlock('/**
$classDescriptionBuilder->addDocBlock('/**
* @psalm-immutable
*/');

Expand All @@ -95,9 +95,9 @@ public function test_it_should_create_annotated_class(): void
$this->assertInstanceOf(ClassDescription::class, $classDescription);

$this->assertEquals(
'/**
['/**
* @psalm-immutable
*/',
*/'],
$classDescription->getDocBlock()
);
}
Expand Down
4 changes: 2 additions & 2 deletions tests/Unit/Analyzer/ClassDescriptionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function test_should_return_true_if_there_class_is_in_namespace_array():
public function test_should_return_true_if_is_annotated_with(): void
{
$cd = $this->builder
->setDocBlock('/**
->addDocBlock('/**
* @psalm-immutable
*/')
->get();
Expand All @@ -73,7 +73,7 @@ public function test_should_return_true_if_is_annotated_with(): void
public function test_should_return_false_if_not_annotated_with(): void
{
$cd = $this->builder
->setDocBlock('/**
->addDocBlock('/**
* @psalm-immutable
*/')
->get();
Expand Down
41 changes: 41 additions & 0 deletions tests/Unit/Analyzer/FileVisitorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Arkitect\Analyzer\FullyQualifiedClassName;
use Arkitect\CLI\TargetPhpVersion;
use Arkitect\Expression\ForClasses\DependsOnlyOnTheseNamespaces;
use Arkitect\Expression\ForClasses\DocBlockNotContains;
use Arkitect\Expression\ForClasses\Implement;
use Arkitect\Expression\ForClasses\NotHaveDependencyOutsideNamespace;
use Arkitect\Rules\ParsingError;
Expand Down Expand Up @@ -448,4 +449,44 @@ enum Enum

$this->assertCount(1, $violations);
}

public function test_it_parse_docblocks(): void
{
$code = <<< 'EOF'
<?php
namespace Root\Cars;

/**
* @throws Exception
*/
class Bar
{
/**
* @throws ItemNotFound
*/
public function getFoo(): int
{
return 1;
}
}
EOF;

/** @var FileParser $fp */
$fp = FileParserFactory::createFileParser(TargetPhpVersion::create('8.1'));
$fp->parse($code, 'relativePathName');

$cd = $fp->getClassDescriptions();

$violations = new Violations();

$notHaveDependencyOutsideNamespace = new DocBlockNotContains('ItemNotFound');
$notHaveDependencyOutsideNamespace->evaluate($cd[0], $violations, 'we want to add this rule for our software');

$this->assertCount(1, $violations);

$notHaveDependencyOutsideNamespace = new DocBlockNotContains('Exception');
$notHaveDependencyOutsideNamespace->evaluate($cd[0], $violations, 'we want to add this rule for our software');

$this->assertCount(2, $violations);
}
}
6 changes: 3 additions & 3 deletions tests/Unit/Expressions/ForClasses/DocBlockContainsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function test_it_should_return_true_if_contains_doc_block(): void
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$because = 'we want to add this rule for our software';
$violations = new Violations();
Expand All @@ -47,7 +47,7 @@ public function test_it_should_return_true_if_contains_doc_block_without_because
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$violations = new Violations();
$expression->evaluate($classDescription, $violations, '');
Expand All @@ -70,7 +70,7 @@ public function test_it_should_return_false_if_not_contains_doc_block(): void
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$because = 'we want to add this rule for our software';
$violations = new Violations();
Expand Down
6 changes: 3 additions & 3 deletions tests/Unit/Expressions/ForClasses/DocBlockNotContainsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function test_it_should_return_true_if_not_contains_doc_block(): void
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$because = 'we want to add this rule for our software';
$violations = new Violations();
Expand All @@ -47,7 +47,7 @@ public function test_it_should_return_false_if_contains_doc_block_without_becaus
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$violations = new Violations();
$expression->evaluate($classDescription, $violations, '');
Expand All @@ -70,7 +70,7 @@ public function test_it_should_return_false_if_contains_doc_block(): void
null,
false,
false,
'/** */myDocBlock with other information'
['/** */myDocBlock with other information']
);
$because = 'we want to add this rule for our software';
$violations = new Violations();
Expand Down
6 changes: 3 additions & 3 deletions tests/Unit/Expressions/ForClasses/HaveAttributeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function test_it_should_return_true_if_contains_doc_block(): void
null,
false,
false,
'',
[],
[FullyQualifiedClassName::fromString('myAttribute')]
);
$because = 'we want to add this rule for our software';
Expand All @@ -48,7 +48,7 @@ public function test_it_should_return_true_if_contains_doc_block_without_because
null,
false,
false,
'',
[],
[FullyQualifiedClassName::fromString('myAttribute')]
);
$violations = new Violations();
Expand All @@ -72,7 +72,7 @@ public function test_it_should_return_false_if_not_contains_doc_block(): void
null,
false,
false,
'',
[],
[FullyQualifiedClassName::fromString('myAttribute')]
);
$because = 'we want to add this rule for our software';
Expand Down