Skip to content
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

feat(css_formatter): Formatting for border property #1453

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

faultyserver
Copy link
Contributor

Summary

#1285. Continuing on the demonstration of border being the first property to be parsed exactly, this is a demonstration of how the property can be formatted` exactly as well.

Turns out it gets a little complicated! The primary addition here is the new FormatPropertyValueFields struct, which implements all of the logic. Given a list of fields, and optionally a slot map, it takes care of building up the Fill element with all of the values. This is what write_component_value_list was already doing for the generic versions, but for the new exact versions it needs to be a bit more involved (the generic ones will still use that function as the fallback).

When there's no slot_map given, the new struct just iterates the fields and adds them as entries to the Fill element. When there is a slot_map, though, the struct iterates the indices of that map and uses the value at each one as the index to pull from the fields for that position in the list. It also handles skipping over empty and missing fields from the syntax, preventing empty elements from being formatted into the fill and causing double spaces or line breaks accidentally.

The slot_map in this case is not the same as the slot_map that gets built when constructing an AstNode, though! Instead, it's the inverse of that map (i.e., the value at index 0 of the inverted map is the index of the value 0 in the original map). This gives the concrete_order_slot_map(), which is implemented on a new AstNodeSlotMap trait that dynamic nodes implement, and is what gets passed to the formatting struct to use for iteration. More information about how this works can be found in the doc comment for concrete_order_slot_map.

The border formatter uses this new struct to easily implement formatting for the value with unordered fields, like:

write!(
    f,
    [FormatPropertyValueFields::new(&format_args![
        line_width.format(),
        line_style.format(),
        color.format(),
    ])
    .with_slot_map(node.concrete_order_slot_map())]
)

Notice that it uses &format_args! to group the slot map pieces together. This isn't strictly necessary right now, but will probably make things in the future easier to deal with (exactly the reason it's used for things like group and indent now). Also note that it formats all of the fields of the node, not just the unordered ones. That includes any tokens or other ordered fields as well. All of them must be present in the format_args! slice, but the nice thing is that extra formatting can still be applied to each, like wrapping one field with a group to preserve flattening/expansion, or indenting one specific field, or anything else.

One thing that I would like to enable in with that is added in the doc comment for the struct under Grouping Fields (Future). The idea here would be that you could say that two fields in a node get formatted together, no matter what their original ordering was, or to be able to add additional formatting like grouping, line breaks, indents, or whatever else makes sense for the fields being put together. Right now that doesn't work with the slot_map iterator solution, since all of the fields have to be present (the slice of args and the slot map have to have the same length), and iteration would cause the grouped field to be formatted twice: once when it comes up naturally, and again when it gets formatted as part of the group with another field.

I spent quite a while trying to make a clean solution work where the fields were wrapped as an Option type and you could insert EmptyFieldSlot values in the slots where a field was moved away from. Unfortunately, it just doesn't look good at all and would be super verbose, and macros aren't quite powerful enough to make it work efficiently either. Since it's unlikely that we'll even need that behavior for a long time, I figure it's fine to leave out for now.

Test Plan

Added a snapshot test for border that includes various permutations of all the allowed properties, including switching up their ordering, and the output matches exactly as expected, preserving the order from the input source!

Copy link

netlify bot commented Jan 7, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit 93343b5
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/659a24636f8f4e0008f2b3e2

@github-actions github-actions bot added A-Core Area: core A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Jan 7, 2024
Copy link
Contributor

github-actions bot commented Jan 7, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 592 592 0
Failed 70 70 0
Panics 0 0 0
Coverage 89.43% 89.43% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13452 13452 0
Failed 4192 4192 0
Panics 2 2 0
Coverage 76.23% 76.23% 0.00%

Copy link

codspeed-hq bot commented Jan 7, 2024

CodSpeed Performance Report

Merging #1453 will improve performances by 9.2%

Comparing faulty/css-format-border (93343b5) with main (166cbab)

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

Benchmark main faulty/css-format-border Change
router.ts[uncached] 13.7 ms 12.6 ms +9.2%

Copy link
Contributor

@denbezrukov denbezrukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the very detailed docs; they helped a lot in understanding the idea behind the PR!

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's NEAT! Amazing work

@ematipico ematipico merged commit cd9623d into main Jan 16, 2024
21 checks passed
@ematipico ematipico deleted the faulty/css-format-border branch January 16, 2024 15:43
ematipico pushed a commit to DaniGuardiola/biome that referenced this pull request Jan 24, 2024
@faultyserver faultyserver mentioned this pull request Feb 14, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Area: core A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants