From aacce226a0684a3549c6213fa19a69ed1306bf69 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Fri, 18 Nov 2022 23:14:35 +1100 Subject: [PATCH 01/16] fix no shouty constants false error on export object preperty --- .../style/no_shouty_constants.rs | 12 ++++- .../tests/specs/style/noShoutyConstants.js | 11 +++- .../specs/style/noShoutyConstants.js.snap | 50 ++++++++++++++++++- 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 80284c8cdcb..e055a70990c 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -5,8 +5,8 @@ use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, - JsVariableDeclaratorList, + JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarationClause, + JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; @@ -101,6 +101,14 @@ impl Rule for NoShoutyConstants { return None; } + if binding.all_references(ctx.model()).all(|r| { + r.node() + .ancestors() + .any(|n| JsVariableDeclarationClause::can_cast(n.kind())) + }) { + return None; + } + return Some(State { literal, references: binding.all_references(ctx.model()).collect(), diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js index 2a8d045f7fe..5f162be7656 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js @@ -6,6 +6,13 @@ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4"; console.log(FOO, FOO4); let foo = "foo"; +const B = "B"; +export default B; +export const A = "A"; -const A = "A"; -export default A; \ No newline at end of file +const BAR = "BAR"; +const BAR1 = "BAR1"; +console.log(BAR1); +export const bar = { + foo: Bar, +}; diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 08adf2a1f52..94525a2e91d 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -1,5 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs +assertion_line: 73 expression: noShoutyConstants.js --- # Input @@ -12,9 +13,17 @@ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4"; console.log(FOO, FOO4); let foo = "foo"; +const B = "B"; +export default B; +export const A = "A"; + +const BAR = "BAR"; +const BAR1 = "BAR1"; +console.log(BAR1); +export const bar = { + foo: Bar, +}; -const A = "A"; -export default A; ``` # Diagnostics @@ -144,4 +153,41 @@ noShoutyConstants.js:4:34 lint/style/noShoutyConstants FIXABLE ━━━━━ ``` +``` +noShoutyConstants.js:14:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Redundant constant declaration. + + 13 │ const BAR = "BAR"; + > 14 │ const BAR1 = "BAR1"; + │ ^^^^^^^^^^^^^ + 15 │ console.log(BAR1); + 16 │ export const bar = { + + i Used here. + + 13 │ const BAR = "BAR"; + 14 │ const BAR1 = "BAR1"; + > 15 │ console.log(BAR1); + │ ^^^^ + 16 │ export const bar = { + 17 │ foo: Bar, + + i You should avoid declaring constants with a string that's the same + value as the variable name. It introduces a level of unnecessary + indirection when it's only two additional characters to inline. + + i Suggested fix: Use the constant value directly + + 12 12 │ + 13 13 │ const BAR = "BAR"; + 14 │ - const·BAR1·=·"BAR1"; + 15 │ - console.log(BAR1); + 14 │ + console.log("BAR1"); + 16 15 │ export const bar = { + 17 16 │ foo: Bar, + + +``` + From 5cbbb4451f6685f5254547c4ac73084e344d91bf Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sat, 19 Nov 2022 00:10:02 +1100 Subject: [PATCH 02/16] ignore shouty constants when it's used more than once --- .../style/no_shouty_constants.rs | 11 ++++- .../specs/style/noShoutyConstants.js.snap | 48 ------------------- 2 files changed, 9 insertions(+), 50 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index e055a70990c..4786e91c3a0 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -5,8 +5,8 @@ use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarationClause, - JsVariableDeclarator, JsVariableDeclaratorList, + JsReferenceIdentifier, JsStringLiteralExpression, JsVariableDeclaration, + JsVariableDeclarationClause, JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; @@ -108,6 +108,13 @@ impl Rule for NoShoutyConstants { }) { return None; } + // check if constant is used in multiple places + if binding + .all_references(ctx.model()).count() + > 1 + { + return None; + } return Some(State { literal, diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 94525a2e91d..5b79f798fa6 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -27,54 +27,6 @@ export const bar = { ``` # Diagnostics -``` -noShoutyConstants.js:1:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Redundant constant declaration. - - > 1 │ const FOO = "FOO"; - │ ^^^^^^^^^^^ - 2 │ console.log(FOO, FOO2); - 3 │ - - i Used here. - - 1 │ const FOO = "FOO"; - > 2 │ console.log(FOO, FOO2); - │ ^^^ - 3 │ - 4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4"; - - i Used here. - - 4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4"; - 5 │ - > 6 │ console.log(FOO, FOO4); - │ ^^^ - 7 │ - 8 │ let foo = "foo"; - - i You should avoid declaring constants with a string that's the same - value as the variable name. It introduces a level of unnecessary - indirection when it's only two additional characters to inline. - - i Suggested fix: Use the constant value directly - - 1 │ - const·FOO·=·"FOO"; - 2 │ - console.log(FOO,·FOO2); - 1 │ + - 2 │ + console.log("FOO",·FOO2); - 3 3 │ - 4 4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4"; - 5 5 │ - 6 │ - console.log(FOO,·FOO4); - 6 │ + console.log("FOO",·FOO4); - 7 7 │ - 8 8 │ let foo = "foo"; - - -``` - ``` noShoutyConstants.js:4:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ From a32da675576bd4f30236d61ec87f95f1958b79ee Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sat, 19 Nov 2022 00:18:17 +1100 Subject: [PATCH 03/16] move usage check first to improve performance --- .../semantic_analyzers/style/no_shouty_constants.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 4786e91c3a0..bab20c56784 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -101,6 +101,11 @@ impl Rule for NoShoutyConstants { return None; } + // check if constant is used in multiple places + if binding.all_references(ctx.model()).count() > 1 { + return None; + } + if binding.all_references(ctx.model()).all(|r| { r.node() .ancestors() @@ -108,13 +113,6 @@ impl Rule for NoShoutyConstants { }) { return None; } - // check if constant is used in multiple places - if binding - .all_references(ctx.model()).count() - > 1 - { - return None; - } return Some(State { literal, From bfcefc3797ff7c1da82af4a675308780123be0b4 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sat, 19 Nov 2022 00:25:01 +1100 Subject: [PATCH 04/16] fix lint --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index bab20c56784..6990e1e3ca0 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -5,8 +5,8 @@ use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsReferenceIdentifier, JsStringLiteralExpression, JsVariableDeclaration, - JsVariableDeclarationClause, JsVariableDeclarator, JsVariableDeclaratorList, + JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarationClause, + JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; @@ -105,7 +105,7 @@ impl Rule for NoShoutyConstants { if binding.all_references(ctx.model()).count() > 1 { return None; } - + if binding.all_references(ctx.model()).all(|r| { r.node() .ancestors() From ade36ee445cc4f21e68b3862def66460fa7d5de9 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 18:53:43 +1100 Subject: [PATCH 05/16] refactor to use modal exported check --- .../style/no_shouty_constants.rs | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 6990e1e3ca0..ab995510d4a 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -5,8 +5,8 @@ use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarationClause, - JsVariableDeclarator, JsVariableDeclaratorList, + JsReferenceIdentifier, JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, + JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; @@ -97,21 +97,22 @@ impl Rule for NoShoutyConstants { if let Some((binding, literal)) = is_id_and_string_literal_inner_text_equal(declarator) { let model = ctx.model(); - if model.is_exported(&binding) { - return None; - } - // check if constant is used in multiple places - if binding.all_references(ctx.model()).count() > 1 { - return None; + if binding.all_references(ctx.model()).count() >1 { + return None } - if binding.all_references(ctx.model()).all(|r| { - r.node() - .ancestors() - .any(|n| JsVariableDeclarationClause::can_cast(n.kind())) - }) { - return None; + for reference in binding.all_references(ctx.model()) { + if let Some(js_reference_identifier) = + JsReferenceIdentifier::cast_ref(reference.node()) + { + if model + .is_exported(&js_reference_identifier) + .unwrap_or_default() + { + return None; + } + } } return Some(State { From c3a5fe6b370e7f3a7dbbbe8b8bf9e31420c1c9b9 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 18:57:41 +1100 Subject: [PATCH 06/16] fix format --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index ab995510d4a..3d047ebb3ed 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -98,12 +98,12 @@ impl Rule for NoShoutyConstants { { let model = ctx.model(); - if binding.all_references(ctx.model()).count() >1 { - return None + if binding.all_references(ctx.model()).count() > 1 { + return None; } for reference in binding.all_references(ctx.model()) { - if let Some(js_reference_identifier) = + if let Some(js_reference_identifier) = JsReferenceIdentifier::cast_ref(reference.node()) { if model From 55171f87ca65e7c977db871b9ddd8583632f1e41 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 19:42:07 +1100 Subject: [PATCH 07/16] fix tests --- .../style/no_shouty_constants.rs | 17 ++----- .../tests/specs/style/noShoutyConstants.js | 8 ++-- .../specs/style/noShoutyConstants.js.snap | 44 ++----------------- 3 files changed, 10 insertions(+), 59 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 3d047ebb3ed..9d482c7e8e2 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -97,24 +97,13 @@ impl Rule for NoShoutyConstants { if let Some((binding, literal)) = is_id_and_string_literal_inner_text_equal(declarator) { let model = ctx.model(); - - if binding.all_references(ctx.model()).count() > 1 { + if model.is_exported(&binding) { return None; } - for reference in binding.all_references(ctx.model()) { - if let Some(js_reference_identifier) = - JsReferenceIdentifier::cast_ref(reference.node()) - { - if model - .is_exported(&js_reference_identifier) - .unwrap_or_default() - { - return None; - } - } + if binding.all_references(ctx.model()).count() > 1 { + return None; } - return Some(State { literal, references: binding.all_references(ctx.model()).collect(), diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js index 5f162be7656..1d38b1a6a6f 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js @@ -11,8 +11,8 @@ export default B; export const A = "A"; const BAR = "BAR"; -const BAR1 = "BAR1"; -console.log(BAR1); + export const bar = { - foo: Bar, -}; + foo: BAR, + bar: BAR, +}; \ No newline at end of file diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 5b79f798fa6..2e55d0b6184 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -18,12 +18,11 @@ export default B; export const A = "A"; const BAR = "BAR"; -const BAR1 = "BAR1"; -console.log(BAR1); + export const bar = { - foo: Bar, + foo: BAR, + bar: BAR, }; - ``` # Diagnostics @@ -105,41 +104,4 @@ noShoutyConstants.js:4:34 lint/style/noShoutyConstants FIXABLE ━━━━━ ``` -``` -noShoutyConstants.js:14:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! Redundant constant declaration. - - 13 │ const BAR = "BAR"; - > 14 │ const BAR1 = "BAR1"; - │ ^^^^^^^^^^^^^ - 15 │ console.log(BAR1); - 16 │ export const bar = { - - i Used here. - - 13 │ const BAR = "BAR"; - 14 │ const BAR1 = "BAR1"; - > 15 │ console.log(BAR1); - │ ^^^^ - 16 │ export const bar = { - 17 │ foo: Bar, - - i You should avoid declaring constants with a string that's the same - value as the variable name. It introduces a level of unnecessary - indirection when it's only two additional characters to inline. - - i Suggested fix: Use the constant value directly - - 12 12 │ - 13 13 │ const BAR = "BAR"; - 14 │ - const·BAR1·=·"BAR1"; - 15 │ - console.log(BAR1); - 14 │ + console.log("BAR1"); - 16 15 │ export const bar = { - 17 16 │ foo: Bar, - - -``` - From 3981559d1e8659e86c80be4ba325dd2ae3f8a3a7 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 19:44:58 +1100 Subject: [PATCH 08/16] fix lint --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 9d482c7e8e2..cec8e1ca8c3 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -4,8 +4,7 @@ use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ - JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsReferenceIdentifier, JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, + JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; From ce35c77fe9fa1f62887c05a751246acb74915094 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 19:49:03 +1100 Subject: [PATCH 09/16] fix format --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index cec8e1ca8c3..173fe0ec191 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -4,7 +4,8 @@ use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ - JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, + JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, + JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; From 3eb24f0d709ef1725a6665e5aa96aef45739cfa4 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 20:01:22 +1100 Subject: [PATCH 10/16] add more test cases --- .../tests/specs/style/noShoutyConstants.js | 9 +++- .../specs/style/noShoutyConstants.js.snap | 47 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js index 1d38b1a6a6f..deb311209dd 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js @@ -15,4 +15,11 @@ const BAR = "BAR"; export const bar = { foo: BAR, bar: BAR, -}; \ No newline at end of file +}; + +const C = "C"; + +export const d = { + foo: C +}; + diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 2e55d0b6184..7c2d4b867ab 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -23,6 +23,14 @@ export const bar = { foo: BAR, bar: BAR, }; + +const C = "C"; + +export const d = { + foo: C +}; + + ``` # Diagnostics @@ -104,4 +112,43 @@ noShoutyConstants.js:4:34 lint/style/noShoutyConstants FIXABLE ━━━━━ ``` +``` +noShoutyConstants.js:20:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Redundant constant declaration. + + 18 │ }; + 19 │ + > 20 │ const C = "C"; + │ ^^^^^^^ + 21 │ + 22 │ export const d = { + + i Used here. + + 22 │ export const d = { + > 23 │ foo: C + │ ^ + 24 │ }; + 25 │ + + i You should avoid declaring constants with a string that's the same + value as the variable name. It introduces a level of unnecessary + indirection when it's only two additional characters to inline. + + i Suggested fix: Use the constant value directly + + 18 18 │ }; + 19 19 │ + 20 │ - const·C·=·"C"; + 21 │ - + 22 20 │ export const d = { + 23 │ - → foo:·C + 21 │ + → foo:·"C" + 24 22 │ }; + 25 23 │ + + +``` + From 48daa78104461716aef52b15ae80d9abeac2b707 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Sun, 20 Nov 2022 22:28:43 +1100 Subject: [PATCH 11/16] skip shorthand property and add test cases --- .../style/no_shouty_constants.rs | 24 ++++++++++++++++--- .../tests/specs/style/noShoutyConstants.js | 4 ++++ .../specs/style/noShoutyConstants.js.snap | 5 +++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 173fe0ec191..ef48476354c 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -5,8 +5,8 @@ use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator, - JsVariableDeclaratorList, + JsReferenceIdentifier, JsShorthandPropertyObjectMember, JsStringLiteralExpression, + JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; @@ -101,9 +101,27 @@ impl Rule for NoShoutyConstants { return None; } - if binding.all_references(ctx.model()).count() > 1 { + if binding.all_references(model).count() > 1 { return None; } + + // skip export as js object shorthand property + for reference in binding.all_references(model) { + if let Some(js_reference_identifier) = + JsReferenceIdentifier::cast_ref(reference.node()) + { + if js_reference_identifier + .syntax() + .parent() + .map_or(false, |n| { + JsShorthandPropertyObjectMember::can_cast(n.kind()) + }) + { + return None; + } + } + } + return Some(State { literal, references: binding.all_references(ctx.model()).collect(), diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js index deb311209dd..983754058f8 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js @@ -23,3 +23,7 @@ export const d = { foo: C }; +const D ="D"; +export const e = { + D +}; \ No newline at end of file diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 7c2d4b867ab..488a313c47e 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -30,7 +30,10 @@ export const d = { foo: C }; - +const D ="D"; +export const e = { + D +}; ``` # Diagnostics From 2169093e1b60135f7b1df0977b112c7d796c4f88 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Tue, 22 Nov 2022 23:50:49 +1100 Subject: [PATCH 12/16] add action for shorthand property object member --- .../style/no_shouty_constants.rs | 64 ++++++++++--------- .../specs/style/noShoutyConstants.js.snap | 39 +++++++++++ 2 files changed, 74 insertions(+), 29 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index ef48476354c..010ec36fadc 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -2,13 +2,15 @@ use crate::{semantic_services::Semantic, utils::batch::JsBatchMutation, JsRuleAc use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Rule, RuleDiagnostic}; use rome_console::markup; use rome_diagnostics::Applicability; +use rome_js_factory::make::{js_literal_member_name, js_property_object_member}; use rome_js_semantic::{AllReferencesExtensions, Reference}; use rome_js_syntax::{ - JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression, - JsReferenceIdentifier, JsShorthandPropertyObjectMember, JsStringLiteralExpression, - JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, + JsAnyExpression, JsAnyLiteralExpression, JsAnyObjectMemberName, JsIdentifierBinding, + JsIdentifierExpression, JsReferenceIdentifier, JsShorthandPropertyObjectMember, + JsStringLiteralExpression, JsSyntaxKind, JsVariableDeclaration, JsVariableDeclarator, + JsVariableDeclaratorList, }; -use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast}; +use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast, SyntaxToken}; declare_rule! { /// Disallow the use of constants which its value is the upper-case version of its name. @@ -41,7 +43,7 @@ declare_rule! { /// ``` /// pub(crate) NoShoutyConstants { - version: "0.7.0", + version: "0.7.1", name: "noShoutyConstants", recommended: true, } @@ -105,23 +107,6 @@ impl Rule for NoShoutyConstants { return None; } - // skip export as js object shorthand property - for reference in binding.all_references(model) { - if let Some(js_reference_identifier) = - JsReferenceIdentifier::cast_ref(reference.node()) - { - if js_reference_identifier - .syntax() - .parent() - .map_or(false, |n| { - JsShorthandPropertyObjectMember::can_cast(n.kind()) - }) - { - return None; - } - } - } - return Some(State { literal, references: binding.all_references(ctx.model()).collect(), @@ -166,15 +151,36 @@ impl Rule for NoShoutyConstants { batch.remove_js_variable_declarator(ctx.query()); for reference in state.references.iter() { - let node = reference + let literal = literal.clone(); + if let Some(node) = reference.node().parent()?.cast::() { + batch.replace_node( + JsAnyExpression::JsIdentifierExpression(node), + JsAnyExpression::JsAnyLiteralExpression(literal), + ); + } else if let Some(node) = reference .node() .parent()? - .cast::()?; - - batch.replace_node( - JsAnyExpression::JsIdentifierExpression(node), - JsAnyExpression::JsAnyLiteralExpression(literal.clone()), - ); + .cast::() + { + // for replacing JsShorthandPropertyObjectMember + let new_element = js_property_object_member( + JsAnyObjectMemberName::JsLiteralMemberName(js_literal_member_name( + SyntaxToken::new_detached( + JsSyntaxKind::JS_LITERAL_MEMBER_NAME, + JsReferenceIdentifier::cast_ref(reference.node())? + .value_token() + .ok()? + .text(), + [], + [], + ), + )), + SyntaxToken::new_detached(JsSyntaxKind::COLON, ":", [], []), + JsAnyExpression::JsAnyLiteralExpression(literal), + ); + + batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into()); + } } Some(JsRuleAction { diff --git a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap index 488a313c47e..2d72e58a160 100644 --- a/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap +++ b/crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap @@ -154,4 +154,43 @@ noShoutyConstants.js:20:7 lint/style/noShoutyConstants FIXABLE ━━━━━ ``` +``` +noShoutyConstants.js:26:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Redundant constant declaration. + + 24 │ }; + 25 │ + > 26 │ const D ="D"; + │ ^^^^^^ + 27 │ export const e = { + 28 │ D + + i Used here. + + 26 │ const D ="D"; + 27 │ export const e = { + > 28 │ D + │ ^ + 29 │ }; + + i You should avoid declaring constants with a string that's the same + value as the variable name. It introduces a level of unnecessary + indirection when it's only two additional characters to inline. + + i Suggested fix: Use the constant value directly + + 23 23 │ foo: C + 24 24 │ }; + 25 │ - + 26 │ - const·D·="D"; + 27 25 │ export const e = { + 28 │ - → D + 26 │ + → + 27 │ + → D:"D" + 29 28 │ }; + + +``` + From 826d4c118471d545af1efb6c3727d294729ec1ba Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Tue, 22 Nov 2022 23:58:35 +1100 Subject: [PATCH 13/16] revert version change --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 010ec36fadc..0bf94e57ebf 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -43,7 +43,7 @@ declare_rule! { /// ``` /// pub(crate) NoShoutyConstants { - version: "0.7.1", + version: "0.7.0", name: "noShoutyConstants", recommended: true, } From b66d94276679203f73c3674443b0702dd6c45e36 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 23 Nov 2022 19:47:33 +1100 Subject: [PATCH 14/16] refactor references --- .../style/no_shouty_constants.rs | 75 ++++++++++--------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 0bf94e57ebf..792f4a853da 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -80,7 +80,7 @@ fn is_id_and_string_literal_inner_text_equal( pub struct State { literal: JsStringLiteralExpression, - references: Vec, + reference: Reference, } impl Rule for NoShoutyConstants { @@ -109,7 +109,7 @@ impl Rule for NoShoutyConstants { return Some(State { literal, - references: binding.all_references(ctx.model()).collect(), + reference: binding.all_references(ctx.model()).next()?, }); } } @@ -128,10 +128,8 @@ impl Rule for NoShoutyConstants { }, ); - for reference in state.references.iter() { - let node = reference.node(); - diag = diag.detail(node.text_trimmed_range(), "Used here.") - } + let node = state.reference.node(); + diag = diag.detail(node.text_trimmed_range(), "Used here."); let diag = diag.note( markup! {"You should avoid declaring constants with a string that's the same @@ -150,37 +148,40 @@ impl Rule for NoShoutyConstants { batch.remove_js_variable_declarator(ctx.query()); - for reference in state.references.iter() { - let literal = literal.clone(); - if let Some(node) = reference.node().parent()?.cast::() { - batch.replace_node( - JsAnyExpression::JsIdentifierExpression(node), - JsAnyExpression::JsAnyLiteralExpression(literal), - ); - } else if let Some(node) = reference - .node() - .parent()? - .cast::() - { - // for replacing JsShorthandPropertyObjectMember - let new_element = js_property_object_member( - JsAnyObjectMemberName::JsLiteralMemberName(js_literal_member_name( - SyntaxToken::new_detached( - JsSyntaxKind::JS_LITERAL_MEMBER_NAME, - JsReferenceIdentifier::cast_ref(reference.node())? - .value_token() - .ok()? - .text(), - [], - [], - ), - )), - SyntaxToken::new_detached(JsSyntaxKind::COLON, ":", [], []), - JsAnyExpression::JsAnyLiteralExpression(literal), - ); - - batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into()); - } + if let Some(node) = state + .reference + .node() + .parent()? + .cast::() + { + batch.replace_node( + JsAnyExpression::JsIdentifierExpression(node), + JsAnyExpression::JsAnyLiteralExpression(literal), + ); + } else if let Some(node) = state + .reference + .node() + .parent()? + .cast::() + { + // for replacing JsShorthandPropertyObjectMember + let new_element = js_property_object_member( + JsAnyObjectMemberName::JsLiteralMemberName(js_literal_member_name( + SyntaxToken::new_detached( + JsSyntaxKind::JS_LITERAL_MEMBER_NAME, + JsReferenceIdentifier::cast_ref(state.reference.node())? + .value_token() + .ok()? + .text(), + [], + [], + ), + )), + SyntaxToken::new_detached(JsSyntaxKind::COLON, ":", [], []), + JsAnyExpression::JsAnyLiteralExpression(literal), + ); + + batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into()); } Some(JsRuleAction { From 27ecf7655bf94aa30a176ae2e5ac781e90417b3d Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 23 Nov 2022 20:34:47 +1100 Subject: [PATCH 15/16] handle default case for action --- .../src/semantic_analyzers/style/no_shouty_constants.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 792f4a853da..e101074069f 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -182,6 +182,8 @@ impl Rule for NoShoutyConstants { ); batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into()); + } else { + return None; } Some(JsRuleAction { From 583b2601eb0996a013a34b1a0ee320f4adc33c21 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Thu, 24 Nov 2022 21:33:27 +1100 Subject: [PATCH 16/16] fix compile error --- .../semantic_analyzers/style/no_shouty_constants.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index bfcb00b5981..e0d72eb64c2 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -109,7 +109,7 @@ impl Rule for NoShoutyConstants { return Some(State { literal, - references: binding.all_references(model).next()?, + reference: binding.all_references(model).next()?, }); } } @@ -128,7 +128,7 @@ impl Rule for NoShoutyConstants { }, ); - let node = state.reference.node(); + let node = state.reference.syntax(); diag = diag.detail(node.text_trimmed_range(), "Used here."); let diag = diag.note( @@ -150,7 +150,7 @@ impl Rule for NoShoutyConstants { if let Some(node) = state .reference - .node() + .syntax() .parent()? .cast::() { @@ -160,7 +160,7 @@ impl Rule for NoShoutyConstants { ); } else if let Some(node) = state .reference - .node() + .syntax() .parent()? .cast::() { @@ -169,7 +169,7 @@ impl Rule for NoShoutyConstants { JsAnyObjectMemberName::JsLiteralMemberName(js_literal_member_name( SyntaxToken::new_detached( JsSyntaxKind::JS_LITERAL_MEMBER_NAME, - JsReferenceIdentifier::cast_ref(state.reference.node())? + JsReferenceIdentifier::cast_ref(state.reference.syntax())? .value_token() .ok()? .text(),