feat(format/html): add multiline for multi attributes #4972
Conversation
Co-authored-by: Nicolas Hedger <[email protected]> Co-authored-by: Arend van Beelen jr. <[email protected]>
Geun-Oh
changed the title
| <div foo="bar" id="foo"></div> | ||
| <div foo="bar"></div> |
There was a problem hiding this comment.
Was this change intentional? Is there a reason?
There was a problem hiding this comment.
Check for single attribute element.
Or should I add new test case?
There was a problem hiding this comment.
You could add new tags/lines to test the different cases
There was a problem hiding this comment.
Got it.
Since the file is for 'no-break' cases, i'd suggesting adding <div foo="bar" id="foo"></div to break.html and
<div foo="bar"></div> to 'no-break.html`.
CodSpeed Performance ReportMerging #4972 will improve performances by 8.23%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
this test case has the setting set to auto. add new tests in a new folder with an options.json to set the attibute multiline setting to multiline. See #4968 for an example of how to do this.
There was a problem hiding this comment.
Thanks. I've added code lines to override options from options.json.
And also add tests & snapshots in new dir multiline.
Co-authored-by: Carson McManus <[email protected]>
This reverts commit a85bb75.
| let options_path = Path::new(test_directory).join("options.json"); | ||
| if options_path.exists() { | ||
| let mut options_path = BiomePath::new(&options_path); | ||
|
|
||
| let mut settings = Settings::default(); | ||
| let (test_options, diagnostics) = deserialize_from_str::<PartialConfiguration>( | ||
| options_path.get_buffer_from_file().as_str(), | ||
| ) | ||
| .consume(); | ||
|
|
||
| settings.merge_with_configuration(test_options.unwrap_or_default(), None, None, &[]).unwrap(); | ||
|
|
||
| let settings = settings.formatter; | ||
|
|
||
| if let Some(attribute_position) = settings.attribute_position { | ||
| options = options.with_attribute_position(attribute_position); | ||
| } | ||
|
|
||
| if !diagnostics.is_empty() { | ||
| for diagnostic in diagnostics { | ||
| println!("{:?}", print_diagnostic_to_string(&diagnostic)); | ||
| } | ||
|
|
||
| panic!("Configuration is invalid"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This logic you added is already handled by SpecTestFile::test.
There was a problem hiding this comment.
Thanks.
There was a missing configuration resolution logic in html.rs that caused attribute_position not to be overridden by test_options. So I added code about attribute_position in html.rs.
This reverts commit 7f759cf.
|
@Geun-Oh can you please direct your PR against We also updated our contribution guide regarding changelogs https://github.com/biomejs/biome/blob/next/CONTRIBUTING.md#changelog |
|
Oh..that's my mistake. |
|
Opps, the branch was deleted temporary.. |
Summary
Close: #4828
Add break if elem has multi attributes (at
multilineside).Test Plan
Modified test case for element with single attribute.