FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[BUGFIX] Fix comment parsing to support multiple comments by ziegenberg · Pull Request #663 · MyIntervals/PHP-CSS-Parser · GitHub

  1. MyIntervals
  2. PHP-CSS-Parser

Repository navigation

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

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

Open
ziegenberg wants to merge 1 commit into
base: main
Choose a base branch
Could not load branches
Branch not found: {{ refName }}

Are you sure you want to change the base?

Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
from
Conversation 22 Commits 1 Checks 0 Files changed
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 .php  (3) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
12 changes: 10 additions & 2 deletions src/Parsing/ParserState.php
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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.

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ziegenberg Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ziegenberg Aug 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, exactly!

ziegenberg Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ziegenberg Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

oliverklee Aug 25, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oliverklee Aug 25, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1161,13 +1161,16 @@ public function commentExtracting(): void
*/
public function flatCommentExtracting(): void

oliverklee Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL