php-parser icon indicating copy to clipboard operation
php-parser copied to clipboard

Inline comments include line break

Open mgrip opened this issue 7 years ago • 9 comments

Seems like when inline comments are parsed they include the line break - both in the comment.value and the comment.loc.end

image

mgrip avatar Apr 26 '18 18:04 mgrip

Not sure that's a bug as the original PHP token used for inline comments also includes the line break. The fix would be counter intuitive, to be clean it should be changed from the tokeniser.

How does it breaks something on prettier ?

ichiriac avatar Apr 26 '18 18:04 ichiriac

We got around it for now by manually stripping out the line break and updating the end location (here). For reference this is the same thing in the JS parser: https://astexplorer.net/ - you can see it doesn't include the new line. Basically it throws off how core prettier assigns comments. It thinks its a leading comment for the newline instead of a trailing comment for the actual line it was written on

mgrip avatar Apr 26 '18 18:04 mgrip

I'm fine keeping as is for now since our workaround seems pretty safe and not all that complicated. Just wanted to flag in case its something you think should actually be changed in the parser

mgrip avatar Apr 26 '18 18:04 mgrip

Ok, got it. In fact I try to stick to original PHP behavior, and sometimes they make strange decisions.

BTW, don't forget to strip mac & windows returns, just add another check after the first if over "\r" - so you will handle "\n", "\r\n", "\r"

I'll try to look if I can do something directly into the AST even if it would be inconsistent with tokens.

ichiriac avatar Apr 26 '18 19:04 ichiriac

Ooh good call, I'll update that. And sounds good, thanks for looking into it!

mgrip avatar Apr 26 '18 20:04 mgrip

Hi @mgrip - does it fixed ? If not, it may introduce breaking changes ? If yes, it should be released on 3.0.0

ichiriac avatar Jan 06 '20 18:01 ichiriac

Thanks for following up @ichiriac! I've since stepped back from the prettier php project, I'll defer to @evilebottnawi or @czosel!

mgrip avatar Jan 06 '20 20:01 mgrip

I tried following up on this a little, but can't report anything conclusive. As far as I can tell, we're ok with the way things are currently handled, so I wouldn't prioritize this for 3.0.0.

czosel avatar Jan 06 '20 21:01 czosel

ok @czosel, thx for the feedback

ichiriac avatar Jan 08 '20 10:01 ichiriac