Skip to content

Conversation

popy-dev
Copy link
Contributor

This is an attempt to fix issue #49. While fixing it it appeared simpler to update the preg split pattern to match (and ignore) whitespaces everywhere, which means following types will work :

  • array<int, string>
  • array < int , string >

If it is too open, I can change that to only ignore one|all whitespace after the , token.

I also updated the PREG comment.

@ashnazg
Copy link
Member

ashnazg commented Aug 9, 2018

@popy-dev I think we do want you to restrict this to one space after comma, while not allowing for any additional spaces elsewhere. Also, you may need to rebase against master to clear the conflict.

@ashnazg
Copy link
Member

ashnazg commented Aug 9, 2018

@popy-dev would also be good to adjust test cases to highlight instances of the extra spacing not being allowed.

@popy-dev
Copy link
Contributor Author

Back from holidays, I'll have a look at this soon. Thanks for feedback !

@popy-dev
Copy link
Contributor Author

popy-dev commented Mar 8, 2019

Maybe i should have answered there after I submitted fixes ?

Also, note that travis runs had bugs unrelated to the patch.

@jaapio
Copy link
Member

jaapio commented Mar 14, 2019

Thanks for the update.I need to check this in detail. To see what is going on. And if all changes are needed.

@jaapio jaapio merged commit bba8c4b into phpDocumentor:master Mar 29, 2019
@jaapio
Copy link
Member

jaapio commented Mar 29, 2019

Thanks, this looks good.

@popy-dev popy-dev deleted the ignore-whitespaces branch July 8, 2019 14:46
@bendavies
Copy link

bendavies commented Sep 20, 2019

this change seems mostly pointless, as https://github.com/phpDocumentor/ReflectionDocBlock splits on whitespace when parsing out @var and @return tags: https://github.com/phpDocumentor/ReflectionDocBlock/blob/master/src/DocBlock/Tags/Var_.php#L56
so for example, given @var Collection<int, Foo>, TypeResolver::resolve('Collection<int') will be called, which will then error.

@jaapio
Copy link
Member

jaapio commented Sep 23, 2019

could be, but that needs to be fixed in the Docblock component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants