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

feat(rome_js_formatter, rome_cli): add arrowParentheses option #4667

Merged
merged 7 commits into from
Jul 17, 2023
Merged

feat(rome_js_formatter, rome_cli): add arrowParentheses option #4667

merged 7 commits into from
Jul 17, 2023

Conversation

SuperchupuDev
Copy link
Contributor

@SuperchupuDev SuperchupuDev commented Jul 7, 2023

Summary

Closes #4666

Personally, the only thing that's keeping me for migrating to Rome formatter-wise is the lack of an arrowParens formatter option. Most developers don't really want to change their code style just to migrate to a new tool, especially when this change affects code readability.

This pull requests adds an --arrow-parentheses option (similar to prettier's arrowParens) that can be set to:

  • always for always adding parentheses to arrow functions
  • as-needed for only adding parentheses where it's necessary

Test Plan

Made all arrow function tests also run on "arrow_parentheses": "AsNeeded"

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit aa140cb
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64b5041c61e77b0008e87593
😎 Deploy Preview https://deploy-preview-4667--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added A-Formatter Area: formatter A-Project Area: project configuration and loading labels Jul 7, 2023
@SuperchupuDev SuperchupuDev marked this pull request as draft July 7, 2023 11:42
@ematipico
Copy link
Contributor

ematipico commented Jul 8, 2023

For testing, here's a start. We have our arrow function testing suite. What you need to is to create an options.json file in that folder. You can copy the way we do here: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/tests/specs/js/module/no-semi/options.json

Then you'd have to update the testing suite, so it can read the new option. Here is where you can do that.

Once you've done it, run the command just test-crate rome_js_formatter, or run the command cargo t from inside the rome_js_formatter/ folder.

The test suite will update ALL the snapshot tests with a new test case with the new options. There, you can check if the formatting is what you expect to be.

@SuperchupuDev
Copy link
Contributor Author

just updated them, but i'm not sure if i did it correctly, the new tests (or rather outputs) this added look completely the same as the other ones

@github-actions github-actions bot added the A-CLI Area: CLI label Jul 10, 2023
@ematipico
Copy link
Contributor

ematipico commented Jul 10, 2023

just updated them, but i'm not sure if i did it correctly, the new tests (or rather outputs) this added look completely the same as the other ones

I'm not really sure tbh, the code looks good. I would need to debug it. It seems that the option is still not passed/overridden.

@SuperchupuDev SuperchupuDev marked this pull request as ready for review July 16, 2023 11:06
Copy link
Contributor

@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 a fantastic addition! Thank you for implementing it!

Two things before merging the PR:

  • if you can revert changes that belong to the website (only the documentation, the playground is fine). We usually update the docs just before doing the release;
  • if possible, add a new test case in the CLI, where we test that passing the new option yields the expected output. You can use this test as example

Copy link
Contributor

@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.

A lot of users will be happy with this new feature! 🙌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Formatter Area: formatter A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Add arrowParentheses formatter option
2 participants