Conversation
keithamus
left a comment
There was a problem hiding this comment.
Great work on this so far! Amazing to see!
I've got some early feedback which hopefully you'll find useful. I think the feedback largely applies to both gap_rule_list/gap_auto_rule_list so I haven't repeated it for each.
Generally speaking, I think you should look at RepeatFunction and see if we can extend into that, here.
Thanks a lot for the thorough review — really appreciate the time and the clear suggestions! |
| // https://drafts.csswg.org/css-gaps-1/#typedef-gap-auto-rule-list | ||
| // <gap-auto-rule-list> = <gap-rule-or-repeat>#? , <gap-auto-repeat-rule> , <gap-rule-or-repeat>#? | ||
| pub type GapAutoRuleList = Todo; | ||
| // |
| // <gap-rule-or-repeat> = <gap-rule> | <gap-repeat-rule> | ||
| #[derive(Parse, Peek, ToCursors, ToSpan, SemanticEq, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize), serde())] | ||
| #[cfg_attr(feature = "visitable", derive(csskit_derives::Visitable), visit(self))] |
There was a problem hiding this comment.
These still need to visit children I believe. Same for many others:
| #[cfg_attr(feature = "visitable", derive(csskit_derives::Visitable), visit(self))] | |
| #[cfg_attr(feature = "visitable", derive(csskit_derives::Visitable), visit)] |
The general rule is "if a type just holds cursors, it should be visit(self). If it is just a delegate type (like a generic) it should be visit(children), otherwise visit."
| let close = p.parse::<T![')']>()?; | ||
| Ok(Self { name, count, comma, rules, close }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks fairly straightforward (nothing surprising). I'm quite sure GapRepeatRule can derive(Parse) if you add #[atom(CssAtomSet::Repeat)] to the name field.
| impl<'a> Parse<'a> for ColumnRuleStyleValue<'a> { | ||
| fn parse<I>(p: &mut Parser<'a, I>) -> ParseResult<Self> | ||
| where | ||
| I: Iterator<Item = Cursor> + Clone, | ||
| { | ||
| p.parse::<GapRuleList<'a>>().map(Self::GapRuleList) | ||
| } | ||
| } | ||
|
|
||
| impl<'a> Parse<'a> for RowRuleStyleValue<'a> { | ||
| fn parse<I>(p: &mut Parser<'a, I>) -> ParseResult<Self> | ||
| where | ||
| I: Iterator<Item = Cursor> + Clone, | ||
| { | ||
| p.parse::<GapRuleList<'a>>().map(Self::GapRuleList) | ||
| } | ||
| } |
There was a problem hiding this comment.
These aren't quite right I think.
This will cause the RowRuleStyleValue to be:
enum RowRuleStyleValue {
GapRuleList(GapRuleList<'a>),
GapAutoRuleList(GapAutoRuleList<'a>),
}But Parse will only ever return the GapRuleList() variant, rendering the other variant useless.
There are two options here:
- Play around with the syntax optimisation phase to flatten these (take a look at https://github.com/csskit/csskit/blob/main/crates/css_value_definition_parser/src/lib.rs#L356-L520). This will mean that
RowRuleStyleValuewill become a struct overGapRuleList. I think this is probably want we want. - Figure out after parse if
GapRuleListfits theautopart by interrogating its members and see if it belongs inGapRuleListorGapAutoRuleList(probably not what we want).
Related issue: #157