-
Notifications
You must be signed in to change notification settings - Fork 145
Parse simple expressions #389
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?
Changes from 9 commits
908bc46
093c527
f734920
7da6868
8627ef6
184b610
9386fe9
d9f89cc
db4d18b
ca89a71
7aaebb2
abbcc8f
14118a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
|
||
namespace Sabberworm\CSS\Value; | ||
|
||
use Sabberworm\CSS\OutputFormat; | ||
use Sabberworm\CSS\Parsing\ParserState; | ||
|
||
/** | ||
* An `Expression` represents a special kind of value that is comprised of multiple components wrapped in parenthesis. | ||
* Examle `height: (vh - 10);`. | ||
*/ | ||
class Expression extends CSSFunction | ||
{ | ||
/** | ||
* @param ParserState $oParserState | ||
* @param bool $bIgnoreCase | ||
* | ||
* @return Expression | ||
* | ||
* @throws SourceException | ||
* @throws UnexpectedEOFException | ||
* @throws UnexpectedTokenException | ||
*/ | ||
public static function parse(ParserState $oParserState, $bIgnoreCase = false) | ||
{ | ||
$oParserState->consume('('); | ||
$aArguments = self::parseArgs($oParserState); | ||
$mResult = new Expression("", $aArguments, ',', $oParserState->currentLine()); | ||
$oParserState->consume(')'); | ||
return $mResult; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a class-specific unit test for this new class. The overall parser tests are good, but we also want to make sure each individual class behaves as it should. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raxbg, would you be willing to create a corresponding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense. How does that look like: 14118a2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -512,6 +512,20 @@ public function expandShorthands() | |
self::assertSame($sExpected, $oDoc->render()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function parseExpressions() | ||
{ | ||
$oDoc = self::parsedStructureForFile('expressions'); | ||
$sExpected = 'div {height: (vh - 10);}' | ||
. "\n" | ||
. 'div {height: (vh - 10)/2;}' | ||
. "\n" | ||
. 'div {height: max(5,(vh - 10));}'; | ||
self::assertSame($sExpected, $oDoc->render()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
|
@@ -688,8 +702,8 @@ public function calcNestedInFile() | |
public function invalidCalcInFile() | ||
{ | ||
$oDoc = self::parsedStructureForFile('calc-invalid', Settings::create()->withMultibyteSupport(true)); | ||
$sExpected = 'div {} | ||
div {} | ||
$sExpected = 'div {height: calc (25% - 1em);} | ||
div {height: calc (25% - 1em);} | ||
div {} | ||
div {height: -moz-calc;} | ||
div {height: calc;}'; | ||
|
@@ -1240,6 +1254,19 @@ public function lonelyImport() | |
self::assertSame($sExpected, $oDoc->render()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function functionArithmeticInFile() | ||
{ | ||
$oDoc = self::parsedStructureForFile('function-arithmetic', Settings::create()->withMultibyteSupport(true)); | ||
$sExpected = 'div {height: max(300,vh + 10);} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, vh without a number in front is still not valid and I’d prefer it would throw an error in strict mode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, it is invalid, however this seems to be outside of the scope of these changes. I guess we need a separate issue for this. Also is it a concern of the parser to validate the semantic meaning of identifiers it encounters? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree it's not. Moving forwards, we've agreed to replace strict mode parsing with a logging mechanism. And try to replicate what most browsers do when encountering invalid syntax. A unit with no number in front should therefore probably be parsed as zero of that unit. And, as a bonus, a separate PR would log the lack of a number. So, for now, we don't need to worry about strict mode, but should create issues for things that could be logged in future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Looking again, this is syntax, which is a concern of the parser. A unit without a length is syntactically invalid, and should be discarded (along with the entire property declaration). |
||
div {height: max(300,vh - 10);} | ||
div {height: max(300,vh * 10);} | ||
div {height: max(300,vh / 10);}'; | ||
self::assertSame($sExpected, $oDoc->render()); | ||
} | ||
|
||
public function escapedSpecialCaseTokens() | ||
{ | ||
$oDoc = $this->parsedStructureForFile('escaped-tokens'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
div { | ||
height: (vh - 10); | ||
} | ||
|
||
div { | ||
height: (vh - 10) / 2; | ||
} | ||
|
||
div { | ||
height: max(5, (vh - 10)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
div { | ||
height: max(300, vh + 10); | ||
} | ||
div { | ||
height: max(300, vh - 10); | ||
} | ||
div { | ||
height: max(300, vh * 10); | ||
} | ||
div { | ||
height: max(300, vh / 10); | ||
} |
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.
Nice refactoring. Could this be put through as a separate PR?