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

Normalize comment classes #867

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

WinterSilence
Copy link
Contributor

@WinterSilence WinterSilence commented Aug 7, 2022

  • getReformattedText(): fix @return types, remove comment trimming to keep empty lines in comment at begin and end, optimize detection of multi-line comments
  • getShortestWhitespacePrefixLen(): replace float INF to PHP_INT_MAX
  • jsonSerialize(): update @psalm-return and remove redundant @return, fix nodeType

@nikic
Copy link
Owner

nikic commented Aug 7, 2022

Test failures are legitimate.

@WinterSilence
Copy link
Contributor Author

@nikic yes, because

remove comment trimming to keep empty lines in comment at begin and end

If you agree with these changes, I'll update the tests

nikic added a commit that referenced this pull request Aug 7, 2022
In the past, single-line comments were stored together with the
trailing newline. Later we switched to the PHP8 comment
representation, where the trailing newline is not part of the
comment anymore. As such, there is also no need to trim here.

This is split out from GH-867.
@nikic
Copy link
Owner

nikic commented Aug 7, 2022

The change to trimming is fine -- to verify, I've landed it separately as 1f504d2. This was a leftover from an older implementation, when comments were expected to have a trailing newline.

It's some of the other changes that cause problematic changes in formatting.

@WinterSilence
Copy link
Contributor Author

@nikic hm... I'm don't see how other changes can fail tests - content changed only here

lib/PhpParser/Comment.php Outdated Show resolved Hide resolved
lib/PhpParser/Comment.php Outdated Show resolved Hide resolved
lib/PhpParser/Comment.php Show resolved Hide resolved
@WinterSilence
Copy link
Contributor Author

WinterSilence commented Aug 7, 2022

@nikic tests failed because startLine/endLine not correct in current test: $this->startLine === $this->endLine always return true because by default they set as -1 i.e. -1=-1

@WinterSilence WinterSilence requested a review from nikic August 7, 2022 19:24
@WinterSilence
Copy link
Contributor Author

WinterSilence commented Aug 10, 2022

@nikic I'm fixed fake/incorrect data passing to CommentTest tests, but I'm think other failed tests must be fixes in other PR's.

@WinterSilence
Copy link
Contributor Author

WinterSilence commented Sep 13, 2022

@nikic added alternative check to test instances:

|| ($this->endLine === -1 && strpos($this->text, "\n") !== false)

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