feat(rome_json_formatter): implement JSON array formatting #4064

dhirajarun · 2022-12-16T16:50:00Z

Summary

JSON Array Formating #2570

netlify · 2022-12-16T16:50:04Z

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	`65acddc`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/63a31af94955190008d7e3c4

dhirajarun · 2022-12-17T05:52:33Z

Some of the prettier snapshot tests are failing, should I accept those?

… errors Some prettier tests for JSON5 are failing after implementing array formatting in #4064 The tests fail because the formatting returns a `FormatError::SyntaxError` if a mandatory child node is missing but the error is never handled by the JSON formatting. This PR fixes this by * Gracefully handle missing values on the root value by calling into verbatim formatting if that's the case * Use `format_or_verbatim` when formatting values I had to pull out the `format_or_verbatim` from the `rome_js_formatter` and move it to `rome_formatter` so that it can be re-used between js and json formatting.

MichaReiser

Yes. You'll need to update the prettier snapshots. Ideally, most of the output will match prettier precisely after your changes (as all JSON formatting is implemented).

However, you have to wait for #4066 to land because some JSON 5 tests are failing because of a syntax error on the root level.

crates/rome_json_formatter/src/json/value/array_value.rs

… errors Some prettier tests for JSON5 are failing after implementing array formatting in #4064 The tests fail because the formatting returns a `FormatError::SyntaxError` if a mandatory child node is missing but the error is never handled by the JSON formatting. This PR fixes this by * Gracefully handle missing values on the root value by calling into verbatim formatting if that's the case * Use `format_or_verbatim` when formatting values I had to pull out the `format_or_verbatim` from the `rome_js_formatter` and move it to `rome_formatter` so that it can be re-used between js and json formatting.

Co-authored-by: Micha Reiser <micha@reiser.io>

dhirajarun · 2022-12-17T12:12:36Z

Yes. You'll need to update the prettier snapshots. Ideally, most of the output will match prettier precisely after your changes (as all JSON formatting is implemented).

However, you have to wait for #4066 to land because some JSON 5 tests are failing because of a syntax error on the root level.

I'll wait then

… errors (#4066) Some prettier tests for JSON5 are failing after implementing array formatting in #4064 The tests fail because the formatting returns a `FormatError::SyntaxError` if a mandatory child node is missing but the error is never handled by the JSON formatting. This PR fixes this by * Gracefully handle missing values on the root value by calling into verbatim formatting if that's the case * Use `format_or_verbatim` when formatting values I had to pull out the `format_or_verbatim` from the `rome_js_formatter` and move it to `rome_formatter` so that it can be re-used between js and json formatting.

MichaReiser · 2022-12-20T11:02:58Z

The last thing missing after merging main is to use the fill layout if the array only consists of string, number, boolean, or null literals as you can see in this example

The logic should work similar to the JavaScript array formatting

tools/crates/rome_js_formatter/src/js/lists/array_element_list.rs

Lines 28 to 61 in 2655264

    
           let layout = if can_concisely_print_array_list(node, f.context().comments()) { 
        
               ArrayLayout::Fill 
        
           } else { 
        
               ArrayLayout::OnePerLine 
        
           }; 
        
           match layout { 
        
               ArrayLayout::Fill => { 
        
                   let trailing_separator = FormatTrailingComma::ES5.trailing_separator(f.options()); 
        
                   let mut filler = f.fill(); 
        
                   // Using format_separated is valid in this case as can_print_fill does not allow holes 
        
                   for (element, formatted) in node.iter().zip( 
        
                       node.format_separated(",") 
        
                           .with_trailing_separator(trailing_separator) 
        
                           .with_group_id(self.group_id), 
        
                   ) { 
        
                       filler.entry( 
        
                           &format_once(|f| { 
        
                               if get_lines_before(element?.syntax()) > 1 { 
        
                                   write!(f, [empty_line()]) 
        
                               } else { 
        
                                   write!(f, [soft_line_break_or_space()]) 
        
                               } 
        
                           }), 
        
                           &formatted, 
        
                       ); 
        
                   } 
        
                   filler.finish() 
        
               } 
        
               ArrayLayout::OnePerLine => write_array_node(node, f), 
        
           }

dhirajarun · 2022-12-20T16:41:50Z

The last thing missing after merging main is to use the fill layout if the array only consists of string, number, boolean, or null literals as you can see in this example

The logic should work similar to the JavaScript array formatting

tools/crates/rome_js_formatter/src/js/lists/array_element_list.rs

Lines 28 to 61 in 2655264

let layout = if can_concisely_print_array_list(node, f.context().comments()) {

ArrayLayout::Fill

} else {

ArrayLayout::OnePerLine

};

match layout {

ArrayLayout::Fill => {

let trailing_separator = FormatTrailingComma::ES5.trailing_separator(f.options());

let mut filler = f.fill();

// Using format_separated is valid in this case as can_print_fill does not allow holes

for (element, formatted) in node.iter().zip(

node.format_separated(",")

.with_trailing_separator(trailing_separator)

.with_group_id(self.group_id),

) {

filler.entry(

&format_once(|f| {

if get_lines_before(element?.syntax()) > 1 {

write!(f, [empty_line()])

} else {

write!(f, [soft_line_break_or_space()])

}

}),

&formatted,

);

}

filler.finish()

}

ArrayLayout::OnePerLine => write_array_node(node, f),

}

done

crates/rome_json_formatter/src/json/lists/array_element_list.rs

feat(rome_json_formatter): implement JSON array formatting

8e45ada

dhirajarun requested review from ematipico and MichaReiser as code owners December 16, 2022 16:50

dhirajarun added 2 commits December 16, 2022 22:24

fix(rome_json_formatter): fix formatting

dcf3dfa

update does_not_format_ignored_directories snap

6c77822

dhirajarun requested a review from leops as a code owner December 17, 2022 05:37

MichaReiser mentioned this pull request Dec 17, 2022

feat(rome_json_formatter): Fallback to Verbatim for nodes with syntax errors #4066

Merged

2 tasks

MichaReiser reviewed Dec 17, 2022

View reviewed changes

crates/rome_json_formatter/src/json/value/array_value.rs Outdated Show resolved Hide resolved

refactor crates/rome_json_formatter/src/json/value/array_value.rs

7d47d6d

Co-authored-by: Micha Reiser <micha@reiser.io>

fix issue because of last commit

e23ebf9

MichaReiser added A-Formatter Area: formatter L-JSON Language: JSON labels Dec 20, 2022

MichaReiser added this to the Next milestone Dec 20, 2022

MichaReiser mentioned this pull request Dec 20, 2022

JSON Formatting #2570

Closed

6 tasks

dhirajarun added 5 commits December 20, 2022 19:40

Merge branch 'main' into formatter/array

2d7632e

add fill Layout

0e42ee8

update json list snapshots

e793d4e

update prettier json list snapshot

58caa97

fix formatting and linting issue

2f29bff

MichaReiser approved these changes Dec 21, 2022

View reviewed changes

dhirajarun added 2 commits December 21, 2022 18:54

resolve suggestions

8d82946

fix doc

869f991

MichaReiser reviewed Dec 21, 2022

View reviewed changes

crates/rome_json_formatter/src/json/lists/array_element_list.rs Outdated Show resolved Hide resolved

Update crates/rome_json_formatter/src/json/lists/array_element_list.rs

65acddc

MichaReiser merged commit 8d5b2ea into rome:main Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(rome_json_formatter): implement JSON array formatting #4064

feat(rome_json_formatter): implement JSON array formatting #4064

Uh oh!

dhirajarun commented Dec 16, 2022

Uh oh!

netlify bot commented Dec 16, 2022 •

edited

Loading

Uh oh!

dhirajarun commented Dec 17, 2022

Uh oh!

MichaReiser left a comment

Uh oh!

Uh oh!

dhirajarun commented Dec 17, 2022

Uh oh!

MichaReiser commented Dec 20, 2022

Uh oh!

dhirajarun commented Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(rome_json_formatter): implement JSON array formatting #4064

feat(rome_json_formatter): implement JSON array formatting #4064

Uh oh!

Conversation

dhirajarun commented Dec 16, 2022

Summary

Uh oh!

netlify bot commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-rometools canceled.

Uh oh!

dhirajarun commented Dec 17, 2022

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhirajarun commented Dec 17, 2022

Uh oh!

MichaReiser commented Dec 20, 2022

Uh oh!

dhirajarun commented Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Dec 16, 2022 •

edited

Loading