Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_formatter): jsx punctuation #3842

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

denbezrukov
Copy link
Contributor

Summary

Fix #3531 (comment)

Test Plan

cargo test -p rome_js_formatter

@netlify
Copy link

netlify bot commented Nov 24, 2022

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 50e2819
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6382218bd2c49f00083f2903

// ,<div>Second</div>
// </div>
// ```
!next_word.is_next_element_self_closing()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know the best solution how we can check two elements from current. =(

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks reasonable to me or you can either:

  • Roll your own iterator that allows a lookahead of two tokens (storing current, and next, and the iterator)
  • Not use Peekable and instead have three local variables:
let mut last = None;
let mut current = iter.next();
let mut next = iter.next();
let mut next_next = iter.next();

// And then at the end do

last = current;
current = next;
next = next_next;
next_next = iter.next()

This may be bad for performance tough because it requires writing a lot of data (OK, only one next more)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -130,8 +130,46 @@ third = (
```diff
--- Prettier
+++ Rome
@@ -80,15 +80,23 @@

@@ -1,15 +1,12 @@
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's something we don't want to

@@ -82,7 +81,8 @@ impl FormatJsxChildList {
children.pop();
}

let mut children_iter = children.into_iter().peekable();
let mut last: Option<&JsxChild> = None;
let mut children_iter = JsxChildrenIterator::new(children.iter());
Copy link
Contributor

@MichaReiser MichaReiser Nov 27, 2022

Choose a reason for hiding this comment

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

Nit: Use into_iter() so that the elements get disposed while iterating and that next returns owned values

@MichaReiser MichaReiser added A-Formatter Area: formatter L-JavaScript Langauge: JavaScript labels Nov 28, 2022
@MichaReiser MichaReiser added this to the 11.0.0 milestone Nov 28, 2022
@MichaReiser MichaReiser merged commit 76689b2 into rome:main Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter L-JavaScript Langauge: JavaScript
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🐛 Various spacing/line length formatting incompatibilities with Prettier
2 participants