Skip to content

Fix 2914: Merge PHPDoc block tags when inheriting#193

Closed
alexeyinkin wants to merge 7 commits intophpstan:masterfrom
alexeyinkin:bug2914_always-inheritdoc
Closed

Fix 2914: Merge PHPDoc block tags when inheriting#193
alexeyinkin wants to merge 7 commits intophpstan:masterfrom
alexeyinkin:bug2914_always-inheritdoc

Conversation

@alexeyinkin
Copy link
Copy Markdown

This merges var, param, return, throws, and deprecated tags for now.

I resorted to a quicker solution keeping PhpDocBlock and also improved it by storing parent blocks. The problem with getting rid of PhpDocBlock is that is stores the param name mapping. It cannot be stored in ResolvedPhpDocBlock because the mapping describes a unit of inheritance and not a ResolvedPhpDocBlock itself (which could be a result of a merge).

I could not write a test to merge 'deprecated' tags because deprecation rules are in a separate repository and the easiest test would be to break a rule. You may add a test in a more complicated way.

I could not run all the existing tests because:
-- 'vendor/bin/phing tests' hangs at 85% for some reason.
-- 'vendor/bin/phpunit --bootstrap=tests/bootstrap.php tests' for some reason attempts to run phpunit recursively at 62% and dies with 'Cannot open file "tests/bootstrap.php"'

Other that that all tests before the '85%' are green.

@alexeyinkin alexeyinkin changed the title #2914 Merge PHPDoc block tags when inheriting Fix 2914: Merge PHPDoc block tags when inheriting May 5, 2020
@ondrejmirtes
Copy link
Copy Markdown
Member

Hi, the build failure means that somewhere in your changes there's an infinite recursion. You can run PHPUnit with --debug and --verbose to see which test crashes like this.

Try running vendor/bin/phing cs and vendor/bin/phing cs-fix locally to fix all the coding standard issues.

Some of the changes in tests seem unnecessary and unrelated, like:

-namespace ForeachWithComplexValueType;
+namespace ForeachIterableWithComplexValueType;

Please get rid of them. Otherwise, this PR seems promising, although it needs some work :) Thank you and I hope we can sucessfully bring this over the finish line.

<arg value="--tab-width=4"/>
<arg value="--cache=tmp/cache/phpcs"/>
<arg value="--ignore=tests/*/data,tests/*/traits,tests/notAutoloaded,src/Reflection/SignatureMap/functionMap.php"/>
<arg value="--ignore=tests/*/data,tests/*/traits,tests/notAutoloaded,tests/*/cache,src/Reflection/SignatureMap/functionMap.php"/>
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Otherwise it checks local generated files.

* @param array<int, self> $parents
* @param array<int, PhpDocBlock> $parentPhpDocBlocks
*/
private function mergeTags(array $parents, array $parentPhpDocBlocks): void // phpcs:disable
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I haven't seed phpcs:disable in your code but not detecting clones is a known issue with phpcs:
slevomat/coding-standard#642

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't want to use clone anyway. I always want to call constructor explicitly. Thanks.

@ondrejmirtes
Copy link
Copy Markdown
Member

Please rebase the branch so that it's on top of master and there are now merge commits :) Thanks.

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