-
Notifications
You must be signed in to change notification settings - Fork 179
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
Allow whitespace (or nothing) in the initial-value
of @property
with universal syntax
#657
Allow whitespace (or nothing) in the initial-value
of @property
with universal syntax
#657
Conversation
I know that this isn't 100% correct because we _only_ want to do that for the `initial-value` property and not for everything. We also "abuse" the fact that `initial-value:;` results in an `EndOfInput` error and then we conver that to `Whitespace("")`. But the idea is as follows: Instead of skipping whitespace, allow it and even allow "nothing". If we skip whitespace, the current parser expects a valid token but there is none and therefore results in an `EndOfInput`. But in case of `initial-value: ;` or `initial-value:;` these values are allowed. Later, we can "ignore" all the whitespace, and only emit a single one (or nothing) for the `initial-value`.
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.
lgtm, thanks!
src/values/syntax.rs
Outdated
.. | ||
}) => &Token::WhiteSpace(""), | ||
Err(e) => return Err(e.into()), | ||
}, |
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.
I guess as you mentioned we might want this function to require a value in other cases, e.g. when parsing custom at rules (which also use this grammar). One idea would be to move this to property.rs
when parsing the initial-value
property. There is already some special handling for universal syntax strings there.
lightningcss/src/rules/property.rs
Lines 78 to 85 in b543fe7
SyntaxString::Universal => match parser.initial_value { | |
None => None, | |
Some(val) => { | |
let mut input = ParserInput::new(val); | |
let mut parser = Parser::new(&mut input); | |
Some(syntax.parse_value(&mut parser)?) | |
} | |
}, |
so basically something like
match syntax.parse_value(&mut parser) {
Ok(v) => Some(v),
Err(BasicParseError {
kind: BasicParseErrorKind::EndOfInput,
..
}) => Some(ParsedComponent::Token(Token::WhiteSpace(""))),
Err(e) => return Err(e)
}
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.
or maybe better than checking for an error, you could use parser.is_exhausted()
to tell if it's already at the end
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.
Yep, the only problem I see with parser.is_exchausted()
is that it will be true for both initial-value:;
and initial-value: ;
and in the second case we do want to know the space exists or not to be collected.
The nice part I think is that is_exhausted()
would be files if an actual error apart from EndOfInput occured. Therefore we can match all Err
safely, I think.
I've updated the code with your suggestions and with the above as well. I think this looks better than the original version I implemented, does this look good to you too now that it is hoisted?
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.
Wait hold on. I think both :;
and : ;
should be allowed, but maybe they both end up as : ;
. That makes it even simpler.
Let me update the code for that.
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.
Alright updated. Right now both initial-value:;
and initial-value: ;
and even initial-value: ;
should be parseable. But in all these scenarios we now print it as initial-value: ;
.
If you don't agree with this change, we should be able to revert the last commit 👍
This is the better spot because now it is specific to the `initial-value` of an `@property` with Universal syntax. Used `is_exhausted` to know that we are "done" or not, it will skip whitespace so it will be true in both the `initial-value:;` and `initial-value: ;` case. Then we still use `parser.next_including_whitespace()` because we are still interested in the whitespace if there is some (in case of `initial-value: ;`). If there is no whitespace, then it results in an end of input error which we can convert to a `WhiteSpace("")` token. We do capture the error to default to an empty whitespace token, but we can capture _all_ errors safely because if _something_ was actually wrong, the wrapping `parser.is_exhausted()` would not have been ok and therefore false.
8796060
to
803c631
Compare
When declaring a
@property
withsyntax: '*'
, then theinitial-value
can be empty or can containe whitespace. Currently this errors with anUnexpected end of input
.This PR fixes that by allowing the whitespace in the
initial-value
spot.I know it is 100% correct because we do want to allow whitespace but just not print all of it.
syntax.rs
file and it's not only for theinitial-value
but for everything that uses that parser withSyntaxString::Universal
but I'm not 100% sure on how to fix this. A new property on the parser? An additional argument with some options?EndOfInput
error in case ofinitial-value:;
, this definitely feels hacky but I got it to work this way. Catching theEndOfInput
allows us to convert it toWhitespce("")
(also hacky).I started with adding the tests so that we can work from there, then updated the parser and the printer.
Looking forward to feedback and obvious improvements!
Fixes: #654