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

Enhance support for multiple line descriptions #26

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mglaman
Copy link
Contributor

commented Apr 24, 2019

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.

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

I've added a test case which matches a failure from phpstan/phpstan#1983 as well.

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

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?

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I'll wait for you guys to resolve this, @JanTvrdik is the expert in this area :)

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@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

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

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.

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

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.

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

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.

@@ -1538,15 +1595,16 @@ public function provideMultiLinePhpDocData(): array
'/**
*
*
* @param Foo $foo 1st multi world description
* @param Foo $foo 1st multi world description with empty lines

This comment has been minimized.

Copy link
@mglaman

mglaman Apr 25, 2019

Author Contributor

Appended to help differentiate to catch an in-development bug where it was consuming all empty line breaks between the next Lexer::TOKEN_IDENTIFIER

Show resolved Hide resolved src/Parser/PhpDocParser.php Outdated
@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Few more things needs to be done.

  1. compare the parsing logic with phpDocumentor
  2. compare the parsing logic with PhpStorm
  3. 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...)
@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@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.)

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

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

This comment has been minimized.

Copy link

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.

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@goba I think it's better to just try it rather than trying to read the code.

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@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.

mglaman added some commits May 9, 2019

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

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.

@mglaman mglaman changed the title Combine multiple line deprecation messages Enhance support for multiple line descriptions May 9, 2019

@mglaman

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I have always retitled the PR to be more accurate :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.