-
Notifications
You must be signed in to change notification settings - Fork 350
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
Convert Sass to Dart 3 style #2038
Conversation
3a7c041
to
2c34d3d
Compare
9cff4ca
to
6c59213
Compare
@@ -13,7 +13,8 @@ import 'comment.dart'; | |||
import 'style_rule.dart'; | |||
|
|||
/// A statement in a plain CSS syntax tree. | |||
abstract class CssNode extends AstNode { | |||
@sealed |
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.
Just to confirm my own understanding: this uses the @sealed
annotation rather than the sealed
keyword because the former allows extensions within the same package while the latter only allows extensions within the same library, right?
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. See also dart-lang/language#3113
if (rest case var rest?) "${_parenthesizeArgument(rest)}...", | ||
if (keywordRest case var keywordRest?) | ||
"${_parenthesizeArgument(keywordRest)}..." |
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.
Is there something extra that using a case expression gets us here? If not, if (rest != null)
reads cleaner to me.
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: Dart doesn't do type inference on fields, so if (rest != null)
would give an error when using rest
in a non-nullable context. This way, we create a new non-nullable variable so inference isn't necessary. (In other words, it allows us to delete var rest = this.rest
and var keywordRest = this.keywordRest
above.)
right.contents.length > 1); | ||
var rightNeedsParens = switch (right) { | ||
BinaryOperationExpression(:var operator) => | ||
operator.precedence <= operator.precedence && |
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'm a little confused by what's going on here. It seems like we're just comparing operator.precedence
(and on the line below, operator
) to itself.
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.
Oops, yeah, this needs some this.
s
codeUnit == $hash && | ||
|
||
case $backslash && var codeUnit: | ||
case var codeUnit when codeUnit == quote: |
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.
Is there a reason why case quote && var codeUnit
doesn't work here?
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.
You can't reference variables in patterns, only constants. See dart-lang/language#3064
var next = text.codeUnitAt(i + 1); | ||
if (isWhitespace(next) || isHex(next)) { | ||
buffer.writeCharCode($space); | ||
switch (text.codeUnitAt(i)) { |
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.
Given how many var codeUnit
s we need below, would it make more sense to just assign it outside the switch?
|
||
case Value_Value.map: | ||
return value.map.entries.isEmpty | ||
? const SassMap.empty() | ||
: SassMap({ | ||
for (var entry in value.map.entries) | ||
_deprotofy(entry.key): _deprotofy(entry.value) | ||
for (var Value_Map_Entry(:key, :value) in value.map.entries) |
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.
Does the pairs
getter not work here?
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.
No, this isn't a Dart map, it's a custom protocol buffer type. (It's not even a protocol buffer map because it needs to guarantee entry order is preserved.)
See sass/sass-spec#1925