-
Notifications
You must be signed in to change notification settings - Fork 65
Enhance support for multiple line descriptions #26
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
Enhance support for multiple line descriptions #26
Conversation
I've added a test case which matches a failure from phpstan/phpstan#1983 as well.
This needs to be implemented for all tags or for none (i.e. modify parseOptionalDescription
method).
The parsing will fail, if the @deprecated
is NOT the last tag, e.g.
yield [ 'OK with long descriptions', '/** * @deprecated foo * @deprecated bar */', new PhpDocNode([ new PhpDocTagNode( '@deprecated', new DeprecatedTagValueNode('foo') ), new PhpDocTagNode( '@deprecated', new DeprecatedTagValueNode('bar') ), ]), ];
which I think could be refactored to properly implement the \Iterator interface
Yes, but it would be mostly useless. TokenIterator
methods provides much finer and precise control than Iterator
methods. Or is there a benefit I'm missing?
I'll wait for you guys to resolve this, @JanTvrdik is the expert in this area :)
@JanTvrdik in regards to iterator: it just felt hard to have a controlled loop over the tokens. I was constantly pushing the internal index out of bounds and having errors thrown. Iterator would allow us to have valid.
It already implements most of the interface's methods, that is why I suggested
The parsing will fail, if the @deprecated is NOT the last tag, e.g.
I see the failure
'description' => 'text first @deprecated text second'
So the fix should go in parseOptionalDescription itself and not generic for deprecated.
it just felt hard to have a controlled loop over the tokens
But how would Iterator
interface help? You would still be unable to use foreach
(because it triggers rewind
).
I see the failure
I don't understand what you mean. Tags which are in the middle of line are intentionally ignored.
But how would Iterator interface help? You would still be unable to use foreach (because it triggers rewind).
Yeah, I see it doesn't really help. Was looking to leverage a valid
method, but didn't need to.
@JanTvrdik that error was a test result and the current implementation was consuming the next line, forcing the second deprecation to be marked as part of the first.
I've made a push that puts the logic in parseText
to handle multiple lined texts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appended to help differentiate to catch an in-development bug where it was consuming all empty line breaks between the next Lexer::TOKEN_IDENTIFIER
Few more things needs to be done.
- compare the parsing logic with phpDocumentor
- compare the parsing logic with PhpStorm
- test it against large real-world dataset (extracted phpdoc from top ~1000 packages on packagist) with focus on how real people write multi-line descriptions (how do they format it, where do they put new lines and indentations...)
@JanTvrdik I have no problem doing that. I made a commit which takes a lengthy example from a Drupal class, and we heavily rely on phpDoc for our docs. aba3906. Documentation: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Field%21FieldItemInterface.php/function/FieldItemInterface%3A%3Aschema/8.6.x
To be honest, I feel like that is now bleeding outside the scope of this initial PR as no existing test coverage ever handled @return
or @param
with descriptions on a new line (the drupalcs standard.)
For what its worth: I'm fine working with what needs to be done. But per my last commit, it seems a generic baseline test needs to be made against master
for comparison.
goba
commented
May 7, 2019
The title of this pull request should be updated to the latest/current scope. I tried to verify how phpDocumentor parses multiline phpdoc, although I don't expect any surprises there. After all this is just about concatenating all the items... However I only found compiler pass implementations (at https://github.com/phpDocumentor/phpDocumentor2/tree/develop/src/phpDocumentor/Compiler/Pass) and could not find where the actual parsing happens.
@goba I think it's better to just try it rather than trying to read the code.
@JanTvrdik I have added an example and would like your feedback. It highlights other problems in the phpdoc parser itself compared to expectations. I need to know if the scope of this PR is changing or not, because I need to decided if I should change route on using PHPStan or not.
I did some experimenting with https://github.com/phpDocumentor/ReflectionDocBlock, and it does preserve \n
per @JanTvrdik recommendation before. In phpstan/phpstan#1983 we can choose to convert into a single line if we choose (as you've suggested.)
@325fc6414ee15a704a9644211d7c9b1cd560145d fixes problems with lost new lines, bringing it back to match previous parsing and phpDoc.
I have always retitled the PR to be more accurate :)
325fc64
to
882d580
Compare
@ondrejmirtes @JanTvrdik this has been rebased against latest master
It's perfect to my eyes, thank you! I'll merge it, test it and tag it right away!
❤️ thanks!
I didn't want to touch \PHPStan\PhpDocParser\Parser\TokenIterator, which I think could be refactored to properly implement the \Iterator interface. That seemed like a task of its own.
This loops over the tokens passed to
\PHPStan\PhpDocParser\Parser\PhpDocParser::parseDeprecatedTagValue
and builds the entire message.