feat(rome_js_formatter): implement option quote_properties #3065

bhbs · 2022-08-16T10:26:37Z

Summary

This PR implements option ~~preserve_quotes~~ quote_properties following Prettier. (docs)
I referred to --quote-style for the implementation of the option.

Close #2482

Test Plan

Check new snapshots
- crates/rome_js_formatter/tests/specs/js/module/string/properties_quotes.js.snap
- crates/rome_js_formatter/tests/specs/ts/string/parameter_quotes.ts.snap

crates/rome_js_formatter/tests/specs/js/module/assignment/assignment.js.snap

bhbs · 2022-08-16T10:39:03Z

After this PR is merged, I’d like to implement this option to VSCode Extension and playground!

ematipico · 2022-08-16T10:51:49Z

After this PR is merged, I’d like to implement this option to VSCode Extension and playground!

The VSCode extension now always reads the configuration from rome.json. We removed all the beta options. Only the playground needs it

MichaReiser

Overall looking good to me. My main suggestion is to change the option to an enum so that it can be extended in the future

MichaReiser · 2022-08-16T11:08:52Z

crates/rome_js_formatter/src/context.rs

@@ -21,6 +21,9 @@ pub struct JsFormatContext {
    /// The style for quotes. Defaults to double.
    quote_style: QuoteStyle,

+    /// Respect the input use of quotes in object properties.
+    preserve_quotes: bool,


I would prefer an enum over a bool so that it becomes possible to support all variants that prettier supports without a breaking change:

preserve

as-needed (what Rome does today. Default)

consistent (out of scope)

I would also rename the setting to quote-properties to closer match prettier's naming

Thanks for the review. I will change it!

ematipico · 2022-08-16T13:57:51Z

crates/rome_service/src/configuration/javascript.rs

@@ -83,6 +83,7 @@ pub struct JavascriptFormatter {
    /// The style for quotes. Defaults to double.
    #[serde(with = "PlainQuoteStyle")]
    pub quote_style: QuoteStyle,
+    pub preserve_quotes: bool,


Could you add some documentation, please?

ematipico · 2022-08-16T13:59:06Z

crates/rome_service/src/file_handlers/javascript.rs

@@ -36,6 +36,7 @@ use std::fmt::Debug;
 #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
 pub struct JsFormatSettings {
    pub quote_style: Option<QuoteStyle>,
+    pub preserve_quotes: bool,


This information should be wrapped under a Option. This will be useful in the future when serializing the configuration. If it's Option::is_none, it won't be serialized.

ematipico · 2022-08-16T13:59:53Z

website/src/_includes/docs/formatter.md

@@ -58,6 +58,7 @@ OPTIONS:
    --indent-size <number>        If the indentation style is set to spaces, determine how many spaces should be used for indentation (default: 2)
    --line-width <number>         Determine how many characters the formatter is allowed to print in a single line (default: 80)
    --quote-style <single|double> Determine whether the formatter should use single or double quotes for strings (default: double)
+    --preserve-quotes             Determine whether the formatter should preserve quotes in object properties (default: false)


Could you revert this change, please? Usually we update the website right before a release, so we don't document stuff that it's unreleased.

ematipico · 2022-08-16T14:00:12Z

website/src/_includes/docs/project-config.md

+#### `javascript.formatter.preserveQuotes`
+
+Respect the input use of quotes in object properties.
+
+> Default: `false`
+


Same here, this change should be reverted.

bhbs · 2022-08-16T14:30:02Z

@MichaReiser @ematipico
Thank you for reviewing!
I addressed your feedback👍

ematipico

Could you add a new test inside rome_cli/tests/ where we test an error when passing a wrong value for --quote-properties? The test should go inside mod formatter. And we can have a snapshot of it, so we can actually see the error message

ematipico · 2022-08-17T07:49:12Z

crates/rome_service/src/configuration/javascript.rs

+#[derive(Deserialize, Serialize, Debug, Eq, PartialEq)]
+#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
+#[serde(rename_all = "camelCase", remote = "QuoteProperties")]
+pub enum PlainQuoteProperties {
+    AsNeeded,
+    Preserve,
+}
+
+impl Default for PlainQuoteProperties {
+    fn default() -> Self {
+        Self::AsNeeded
+    }
+}


Suggested change

#[derive(Deserialize, Serialize, Debug, Eq, PartialEq)]

#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]

#[serde(rename_all = "camelCase", remote = "QuoteProperties")]

pub enum PlainQuoteProperties {

AsNeeded,

Preserve,

}

impl Default for PlainQuoteProperties {

fn default() -> Self {

Self::AsNeeded

}

}

#[derive(Deserialize, Default, Serialize, Debug, Eq, PartialEq)]

#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]

#[serde(rename_all = "camelCase", remote = "QuoteProperties")]

pub enum PlainQuoteProperties {

#[default]

AsNeeded,

Preserve,

}

We can now use macros to assign defaults to enum!

It may be better to call QuoteStyle::default here to make sure these two are always in sync. Not sure if there's a From implementation that easily allows you to do so.

Sorry, I'm not sure, please tell me how to do it

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

bhbs · 2022-08-17T16:34:23Z

@ematipico
Did you intend like this? a6a89ab

ematipico · 2022-08-17T17:46:38Z

@ematipico
Did you intend like this? a6a89ab

Yes, that's it :)

It's okay if it's empty, I plan to change it soon. At least it's ready and there

ematipico

This is a great contribution! Thank you very much!

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

bhbs added 3 commits August 16, 2022 18:47

feat(rome_js_formatter): implement option preserve_quotes

9f8fd10

test: update snapshots

260d10b

test: add option preserve_quotes

b05812a

bhbs commented Aug 16, 2022

View reviewed changes

crates/rome_js_formatter/tests/specs/js/module/assignment/assignment.js.snap Show resolved Hide resolved

bhbs marked this pull request as ready for review August 16, 2022 10:39

bhbs requested review from leops, ematipico and MichaReiser as code owners August 16, 2022 10:39

bhbs requested a review from a team August 16, 2022 10:39

MichaReiser reviewed Aug 16, 2022

View reviewed changes

ematipico suggested changes Aug 16, 2022

View reviewed changes

bhbs added 3 commits August 16, 2022 23:12

Revert document stuff

6a7d440

fix: bool PreserveQuotes to enum QuoteProperties

9bcc5b9

test: update snapshots

451a8c3

bhbs requested review from MichaReiser and ematipico August 16, 2022 16:09

ematipico reviewed Aug 17, 2022

View reviewed changes

bhbs and others added 4 commits August 17, 2022 22:32

Update crates/rome_service/src/configuration/javascript.rs

16abe9b

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

Merge branch 'main' into feature/preserve-quotes

d5e04e5

test: update snapshots

d498dde

test: add test rome_cli quote_properties_parse_errors_letter_case

a6a89ab

MichaReiser requested a review from ematipico August 17, 2022 16:33

Merge branch 'main' into feature/preserve-quotes

dc1fbd0

MichaReiser approved these changes Aug 18, 2022

View reviewed changes

bhbs changed the title ~~feat(rome_js_formatter): implement option preserve_quotes~~ feat(rome_js_formatter): implement option quote_properties Aug 18, 2022

ematipico approved these changes Aug 18, 2022

View reviewed changes

ematipico merged commit c6d0450 into rome:main Aug 18, 2022

MichaReiser mentioned this pull request Aug 18, 2022

☂️ Documentation changes 0.9.0 #2991

Closed

bhbs deleted the feature/preserve-quotes branch August 20, 2022 02:32

bhbs mentioned this pull request Aug 20, 2022

fix(rome_service): the property for config should be camelCase #3090

Merged

IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022

feat(rome_js_formatter): implement option quote_properties (rome#3065)

36c52c5

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

bhbs mentioned this pull request Aug 28, 2022

feat(playground): implement option quote_properties #3121

Merged

feat(rome_js_formatter): implement option quote_properties #3065

feat(rome_js_formatter): implement option quote_properties #3065

Uh oh!

Conversation

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

Uh oh!

Summary

Test Plan

Uh oh!

Uh oh!

bhbs commented Aug 16, 2022

Uh oh!

ematipico commented Aug 16, 2022

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhbs commented Aug 16, 2022

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhbs commented Aug 17, 2022

Uh oh!

ematipico commented Aug 17, 2022

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhbs commented Aug 16, 2022 •

edited

Loading