Repository navigation
Upgrade to Dart 3.13.0 - #2835
Conversation
jathak
left a comment
jathak
left a comment
There was a problem hiding this comment.
Looking through this, converting everything to primary constructors where possible seems like the right move even if it's sometimes a bit awkward.
| final class ModifiableCssAtRule( | ||
| @override final CssValue<String> name, | ||
| @override final FileSpan span, { | ||
| bool childless = false, |
There was a problem hiding this comment.
Is there a specific reason we use childless for the parameter but isChildless for the field here?
There was a problem hiding this comment.
The style guide says that boolean properties should be verb phrases (like "is childless"), but suggests that as parameters the verb should be omitted.
| required this.parsedAsSassScript, | ||
| FileSpan? valueSpanForMap, | ||
| }) : valueSpanForMap = valueSpanForMap ?? value.span { | ||
| this { |
There was a problem hiding this comment.
I think it might make sense to move constructor bodies to the top of the class when present to avoid the parameter list being separated from the body.
There was a problem hiding this comment.
I understand the impulse, but what used to be "a constructor" is now intrinsically split across multiple locations—the parameters are in the class header, additional field initializers are at the field locations, and the constructor body is at the this call. I think it makes sense to have the constructor body after field initializers, since it's run after they're initialized.
Given that constructor bodies should come after fields, I think it makes sense to put them after properties as well, for the same reason we've historically put constructors after properties: the properties (like fields) provide a view into the "state" of the object from a user's perspective, even if they're computable from other state internally.
| /// {@category Parsing} | ||
| @sealed | ||
| abstract class Expression implements SassNode { | ||
| abstract class Expression() implements SassNode { |
There was a problem hiding this comment.
I believe you'd said you would leave off the primary constructors for classes with implicit zero-argument constructors. Is that not the case here?
There was a problem hiding this comment.
Nope, this is just a mistake 😄.
There was a problem hiding this comment.
The failing tests reminded me why this was necessary: because this class has a factory constructor, it doesn't have an implicit zero-argument constructor. One needs to be declared explicitly.
|
|
||
| /// Creates a parser that parses CSS selectors. | ||
| /// | ||
| /// If [_allowParent] is `false`, this will throw a [SassFormatException] if |
There was a problem hiding this comment.
Not sure if it matters in this case since these docs aren't rendered for sass or sass_api, but when using private named parameters, should the reference be to the version with the underscore (as it appears here) or to the name without an underscore (as they would appear when calling this)?
There was a problem hiding this comment.
Dartdoc complains if it's not the version with the underscore, apparently.
2 participants
This includes reformatting with the new Dart style logic and using a number of new language features.