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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX] Fix comment parsing to support multiple comments #663

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −7
Open

[BUGFIX] Fix comment parsing to support multiple comments #663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/Parsing/ParserState.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,18 +220,26 @@ public function parseCharacter($bIsForIdentifier)
}

/**
* @return array<int, Comment>|void
* Consumes whitespace and comments and returns the comments.
* If $withoutComment is true, only whitespace is consumed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put code in comments in backticks so it gets displayed as code.

*
* @param bool $withoutComment Do not consume comments, only whitespace.
*
* @return array<int, Comment>|void List of comments.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can be even more specific (and omit the void return type, which is incorrect for this method and also is not allowed in union types):

Suggested change
* @return array<int, Comment>|void List of comments.
* @return list<Comment> list of comments

*
* @throws UnexpectedEOFException
* @throws UnexpectedTokenException
*/
public function consumeWhiteSpace(): array
public function consumeWhiteSpace(bool $withoutComment = false): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of boolean function parameters that switch behavior. Instead, please let's have a separate, well-named method, e.g., comsumeWhitespaceWithoutComment or something alike.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can then also move the new method to Rule. (I'm not 100% sure about this and would welcome your thoughts on this, @ziegenberg @sabberworm @JakeQZ.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go even further and rename the current function comsumeWhitespace to comsumeWhitespaceWithComment and create a new function comsumeWhitespace which precisely only consumes white space. I was baffled to see comsumeWhitespace also consume comments and return them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would probably be a breaking change. So, I introduced the boolean function parameter to stay within the current wording (which I deem wrong) and still make the change non-breaking.

I would consider having comsumeWhitespace and a new comsumeWhitespaceWithoutComment a not-so-good solution. That would cement the semantically wrong situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of renaming this method!

Would you be willing to do this as a (or more specific), two pre-patch PRs?

  1. one (to be backported) that renames the method, adds the specific type annotation, and re-adds the old method to redirect to the new method, and marks it as @deprecated will be removed in version 9.0 (including a corresponding line in the changelog
  2. another (not to be backported) that removes the old method (including a corresponding entry in the changelog)

I'd be willing to review these two pre-PRs quickly so we can rebase on them.

We also might want to mark the renamed method as @internal as it needs to be public for technical reasons, but is not expected to be called from outside this library. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of comsumeWhitespaceWithoutComment, having a function name telling you what it does not do. But I'll create the two pre-patch PRs as asked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alle consume* functions are in the class ParserState, so why move it to Rule?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it seems I misread your comment and got confused. 🤯

Okay, so I'm fine with this plan:

I would go even further and rename the current function comsumeWhitespace to comsumeWhitespaceWithComment and create a new function comsumeWhitespace which precisely only consumes white space.

We'll need to do this in a non-breaking way for the parts that we backport, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All consume* functions are in the class ParserState, so why move it to Rule?

I've thought about this some more and now think having it in ParserState is perfectly fine. :-)

{
$aComments = [];
do {
while (\preg_match('/\\s/isSu', $this->peek()) === 1) {
$this->consume(1);
}
if ($withoutComment) {
break;
}
if ($this->oParserSettings->bLenientParsing) {
try {
$oComment = $this->consumeComment();
Expand Down
2 changes: 1 addition & 1 deletion src/Rule/Rule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public static function parse(ParserState $oParserState): Rule
while ($oParserState->comes(';')) {
$oParserState->consume(';');
}
$oParserState->consumeWhiteSpace();
$oParserState->consumeWhiteSpace(true);

return $oRule;
}
Expand Down
11 changes: 7 additions & 4 deletions tests/ParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,13 +1161,16 @@ public function commentExtracting(): void
*/
public function flatCommentExtracting(): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're planning to split the tests and testcases into smaller, more focused tests. To make this easier for us, please don't expand the test. Instead, could you please duplicate it into two separate tests, one with a single comment and one with multiple? Thanks!

{
$parser = new Parser('div {/*Find Me!*/left:10px; text-align:left;}');
$parser = new Parser('div {/*Find Me!*/left:10px; /*Find Me Too!*/text-align:left;}');
$doc = $parser->parse();
$contents = $doc->getContents();
$divRules = $contents[0]->getRules();
$comments = $divRules[0]->getComments();
self::assertCount(1, $comments);
self::assertSame('Find Me!', $comments[0]->getComment());
$rule1Comments = $divRules[0]->getComments();
$rule2Comments = $divRules[1]->getComments();
self::assertCount(1, $rule1Comments);
self::assertCount(1, $rule2Comments);
self::assertEquals('Find Me!', $rule1Comments[0]->getComment());
self::assertEquals('Find Me Too!', $rule2Comments[0]->getComment());
}

/**
Expand Down