Combine parsing into legacy and new parsing. #328
Conversation
There was a problem hiding this comment.
I like the parse_components split. If this PR is on its own was the final state, I'd insist on having a simple parse_components with an allow_legacy argument, same for parse_alpha with uses_commas.
But if you somehow require the split later down the road and this makes it easier for you, then this is fine with the nits below.
| let r2 = f2(color_parser, input)?; | ||
| let r3 = f3(color_parser, input)?; | ||
|
|
||
| let alpha = if !input.is_exhausted() { |
There was a problem hiding this comment.
Seems we could keep having a single parse_alpha, but pass /* uses_commas = */ false here?
There was a problem hiding this comment.
We know what to expect at compile time here, so I like this for performance. Also, when none is implemented, the legacy syntax will return f32, but for new syntax it will be Option<f32>.
| /// Try to parse the components and alpha with the legacy syntax, but also allow | ||
| /// the [color-4] syntax if that fails. | ||
| /// https://drafts.csswg.org/css-color-4/#color-syntax-legacy | ||
| pub fn parse_legacy_components<'i, 't, P, F1, F2, F3, R1, R2, R3>( |
There was a problem hiding this comment.
Maybe parse_components_allow_legacy would be a better name for this?
(But then again we could have a single parse_components with allow_legacy: bool). I assume this makes it easier to handle none in the future somehow tho, so this is fine as an interim step.
There was a problem hiding this comment.
Yes, adding none complicates things, because the legacy syntax doesn't allow none. But also, because if the function supports legacy, then the newer syntax for that same function doesn't allow none either. So adding the allow_legacy turns into an if explosion.
|
As mentioned most of this will be clearer once |
|
@bors-servo r+ Okay, let's try this. if we care about the extra branches when we know we can't parse legacy syntax, I think a template argument But anyways let's do this for now and we can reevaluate as we implement |
|
📌 Commit 85463cd has been approved by |
|
☀️ Test successful - checks-github |
Combine the rules of legacy and new parsing into single functions. This allows for a much clearer separation of which keywords support which syntax. RGB is still special, because it the components must be of the same type (numbers or percentages). Most of the color functions are using the new syntax, but some special notes:
RGBuses it's own parser as mentioned.HSLsupports both legacy and new syntax.HWBonly supports new suntax.These changes are also in preparation of introducing the
nonekeyword where it is much more important to know which syntax is being used, as legacy syntax does not support thenonekeyword.