-
Notifications
You must be signed in to change notification settings - Fork 145
[BUGFIX] Allow comma-separated arguments in selectors #1292
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
base: main
Are you sure you want to change the base?
Conversation
This is rough and ready, though fixes #1289. I have no idea why an additional fix for comment parsing seems to be required. I expect judicious review, and that this PR needs to be split because of the curious comment-parsing bug (why is nothing ever easy?). Anyhow, we're on the road to sorting this. |
8626ec3
to
5705132
Compare
src/Parsing/ParserState.php
Outdated
@@ -345,6 +345,10 @@ public function consumeUntil( | |||
$start = $this->currentPosition; | |||
|
|||
while (!$this->isEnd()) { | |||
$comment = $this->consumeComment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cover this with a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit mysterious why the bug this is fixing suddenly reared it's head with this change. Will need investigating as a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how should we proceed here? Fix that bug first and then continue here? Or merge this PR first and than fix that bug (and cover it with tests)?
I tend to prefer the first, but am not completely sure.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment parsing bugfix seems to be needed for this PR (though I don't yet understand why), so it would need to be done first if it really is required for this change to work.
- Assign the result of `ParserState::peek()` to a local variable, for efficiency; - Use a switch statement to branch on its value, for extensibility (e.g. #1292); - Don't unnecessarily test that a quote character is not escaped when not within a string.
- Assign the result of `ParserState::peek()` to a local variable, for efficiency; - Use a switch statement to branch on its value, for extensibility (e.g. #1292); - Don't unnecessarily test that a quote character is not escaped when not within a string.
This PR now needs a rebase. Apart from that, is it ready for a re-review, or does it need any other changes first? |
Have rebased. The mysterious issue with comment consumption still needs to be resolved... |
About the commit message: The |
It the docs are correct, they can be on the same line, but do need to be written out individually. However, I think it's more readable if they are on separate lines, given the format I originally used won't work. I've amended the original commit. |
I've found that this arose because this PR changes consumption behaviour to consume the first character after the selector separator ( The "Number 4" comment (here) is then the next thing It so happens that there is a space after the comma, which if not pre-consumed, would allow |
|
Also remove fix for `consumeUntil` bug, which #1320 will address.
This is now fixed. I think the I think it would be possible to apply the change regarding |
$parserState->consume(1); | ||
$consumedNextCharacter = true; | ||
} | ||
break; | ||
} | ||
} while (!\in_array($nextCharacter, ['{', '}'], true) || \is_string($stringWrapperCharacter)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isset($stringWrapperCharacter)
and \is_string($stringWrapperCharacter)
are both checking the same thing, right? If so, maybe settle on using one of the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was changed to is_string
in a pre-PR, but I forgot to update it in this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted. It's reassuring to have two reviewers pick up on the same issue :)
src/RuleSet/DeclarationBlock.php
Outdated
$selectorParts[] = $parserState->consume(1); | ||
} | ||
$selectorParts[] = $parserState->consumeUntil( | ||
['{', '}', '\'', '"', '(', ')', ','], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this easier to understand, I propose moving this array either to a class constant or a well-named local variable. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a local variable, as it's only applicable to this method. It also matches all the cases in the switch statement. ({
and }
could be handled as switch cases rather than by the while
, but it's clearer that these are loop end cases the way it's currently written.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/RuleSet/DeclarationBlock.php
Outdated
@@ -58,9 +69,31 @@ public static function parse(ParserState $parserState, ?CSSList $list = null): ? | |||
} | |||
} | |||
break; | |||
case '(': | |||
if (!isset($stringWrapperCharacter)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can convert all those to is_string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I forgot to update this after making the change in the pre-PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Co-authored-by: Oliver Klee <[email protected]>
$subject = DeclarationBlock::parse(new ParserState($selector . '{}', Settings::create())); | ||
|
||
$resultSelectorStrings = \array_map( | ||
static function (Selector $selectorObject): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this two times now - so I'd like to propose we move this to a well-named private method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. As a stickler for code re-use, I can't believe I missed this. Sorted.
I've addressed all comments so far. But I think there's a bit of a "smell" in the tests which have "parses and returns" in a single test.
I'm increasingly thinking this would be a good approach, if only to separate the changes into manageable chunks. |
Fixes #138.
Fixes #360.
Fixes #1289.