From 7e8dfa2ecc61d012686a40f28c5fad6558c5443f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 10 Nov 2022 11:08:28 +0100 Subject: [PATCH] Fix clippy warning. Rename helpers --- .../a11y/use_key_with_click_events.rs | 118 +++++++++--------- crates/rome_js_analyze/src/utils/tests.rs | 2 +- crates/rome_js_syntax/src/jsx_ext.rs | 78 ++++++------ website/docs/src/lint/rules/index.md | 2 +- .../src/lint/rules/useKeyWithClickEvents.md | 6 +- 5 files changed, 100 insertions(+), 106 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs b/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs index e467993ac4d..d16610e8b20 100644 --- a/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs +++ b/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs @@ -1,10 +1,12 @@ use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic}; use rome_console::markup; -use rome_js_syntax::{JsxAnyAttribute, JsxOpeningElement, JsxSelfClosingElement}; -use rome_rowan::{declare_node_union, AstNode, AstNodeList}; +use rome_js_syntax::{ + JsxAnyAttribute, JsxAnyElementName, JsxAttributeList, JsxOpeningElement, JsxSelfClosingElement, +}; +use rome_rowan::{declare_node_union, AstNode, SyntaxResult}; declare_rule! { - /// Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `noKeyPress` keyboard event. + /// Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `onKeyPress` keyboard event. /// /// ## Examples /// @@ -44,6 +46,10 @@ declare_rule! { /// ```jsx ///
{}} >
/// ``` + /// + /// ```jsx + /// + /// ``` pub(crate) UseKeyWithClickEvents { version: "10.0.0", name: "useKeyWithClickEvents", @@ -56,22 +62,19 @@ declare_node_union! { } impl JsxAnyElement { - fn has_spread_attribute(&self) -> bool { + fn attributes(&self) -> JsxAttributeList { match self { - JsxAnyElement::JsxOpeningElement(element) => element - .attributes() - .iter() - .any(|attribute| matches!(attribute, JsxAnyAttribute::JsxSpreadAttribute(_))), - JsxAnyElement::JsxSelfClosingElement(element) => element - .attributes() - .iter() - .any(|attribute| matches!(attribute, JsxAnyAttribute::JsxSpreadAttribute(_))), + JsxAnyElement::JsxOpeningElement(element) => element.attributes(), + JsxAnyElement::JsxSelfClosingElement(element) => element.attributes(), } } -} -impl UseKeyWithClickEvents { - const REQUIRED_PROPS: [&'static str; 3] = ["onKeyDown", "onKeyUp", "onKeyPress"]; + fn name(&self) -> SyntaxResult { + match self { + JsxAnyElement::JsxOpeningElement(element) => element.name(), + JsxAnyElement::JsxSelfClosingElement(element) => element.name(), + } + } } impl Rule for UseKeyWithClickEvents { @@ -81,64 +84,55 @@ impl Rule for UseKeyWithClickEvents { type Options = (); fn run(ctx: &RuleContext) -> Self::Signals { - let node = ctx.query(); + let element = ctx.query(); + + match element.name() { + Ok(JsxAnyElementName::JsxName(name)) => { + let element_name = name.value_token().ok()?.text_trimmed().to_lowercase(); - match node { - JsxAnyElement::JsxOpeningElement(element) => { - let on_click_attribute = element.find_attribute_by_name("onClick").ok()?; - if element.name().ok()?.as_jsx_name().is_none() - || on_click_attribute.is_none() - || node.has_spread_attribute() - { + // Don't handle interactive roles + // TODO Support aria roles https://github.com/rome/tools/issues/3640 + if matches!( + element_name.as_str(), + "button" | "checkbox" | "combobox" | "a" | "input" + ) { return None; } + } + _ => { + return None; + } + } - for attribute in element.attributes().into_iter() { - if let JsxAnyAttribute::JsxAttribute(attribute) = attribute { - let name = attribute - .name() - .ok()? - .as_jsx_name()? - .syntax() - .text_trimmed() - .to_string(); - - if Self::REQUIRED_PROPS.contains(&name.as_str()) { - return None; - } - } - } + let attributes = element.attributes(); + let on_click_attribute = attributes.find_by_name("onClick").ok()?; - Some(()) - } - JsxAnyElement::JsxSelfClosingElement(element) => { - let on_click_attribute = element.find_attribute_by_name("onClick").ok()?; - if element.name().ok()?.as_jsx_name().is_none() - || on_click_attribute.is_none() - || node.has_spread_attribute() - { - return None; - } + #[allow(clippy::question_mark)] + if on_click_attribute.is_none() { + return None; + } - for attribute in element.attributes().into_iter() { - if let JsxAnyAttribute::JsxAttribute(attribute) = attribute { - let name = attribute - .name() - .ok()? - .as_jsx_name()? - .syntax() - .text_trimmed() - .to_string(); + for attribute in attributes { + match attribute { + JsxAnyAttribute::JsxAttribute(attribute) => { + let attribute_name = attribute.name().ok()?; + let name = attribute_name.as_jsx_name()?; + let name_token = name.value_token().ok()?; - if Self::REQUIRED_PROPS.contains(&name.as_str()) { - return None; - } + if matches!( + name_token.text_trimmed(), + "onKeyDown" | "onKeyUp" | "onKeyPress" + ) { + return None; } } - - Some(()) + JsxAnyAttribute::JsxSpreadAttribute(_) => { + return None; + } } } + + Some(()) } fn diagnostic(ctx: &RuleContext, _: &Self::State) -> Option { diff --git a/crates/rome_js_analyze/src/utils/tests.rs b/crates/rome_js_analyze/src/utils/tests.rs index 5ffb9d28623..33011443fc6 100644 --- a/crates/rome_js_analyze/src/utils/tests.rs +++ b/crates/rome_js_analyze/src/utils/tests.rs @@ -116,7 +116,7 @@ pub fn ok_find_attributes_by_name() { .filter_map(rome_js_syntax::JsxAttributeList::cast) .next() .unwrap(); - let [a, c, d] = list.find_attributes_by_name(["a", "c", "d"]); + let [a, c, d] = list.find_by_names(["a", "c", "d"]); assert_eq!( a.unwrap() .initializer() diff --git a/crates/rome_js_syntax/src/jsx_ext.rs b/crates/rome_js_syntax/src/jsx_ext.rs index e7876397df3..9b70c7eb527 100644 --- a/crates/rome_js_syntax/src/jsx_ext.rs +++ b/crates/rome_js_syntax/src/jsx_ext.rs @@ -84,7 +84,7 @@ impl JsxOpeningElement { &self, name_to_lookup: &str, ) -> SyntaxResult> { - find_attribute_by_name(self.attributes(), name_to_lookup) + self.attributes().find_by_name(name_to_lookup) } /// It checks if current attribute has a trailing spread props @@ -131,7 +131,8 @@ impl JsxOpeningElement { /// assert!(opening_element.has_trailing_spread_prop(div.clone())); /// ``` pub fn has_trailing_spread_prop(&self, current_attribute: impl Into) -> bool { - has_trailing_spread_prop(self.attributes(), current_attribute) + self.attributes() + .has_trailing_spread_prop(current_attribute) } } @@ -182,7 +183,7 @@ impl JsxSelfClosingElement { &self, name_to_lookup: &str, ) -> SyntaxResult> { - find_attribute_by_name(self.attributes(), name_to_lookup) + self.attributes().find_by_name(name_to_lookup) } /// It checks if current attribute has a trailing spread props @@ -230,21 +231,22 @@ impl JsxSelfClosingElement { /// assert!(opening_element.has_trailing_spread_prop(div.clone())); /// ``` pub fn has_trailing_spread_prop(&self, current_attribute: impl Into) -> bool { - has_trailing_spread_prop(self.attributes(), current_attribute) + self.attributes() + .has_trailing_spread_prop(current_attribute) } } impl JsxAttributeList { - /// Find and return the `JsxAttribute` that matches the given name like [find_attribute_by_name]. - /// Only attributes with name as [JsxName] can be returned. + /// Finds and returns attributes `JsxAttribute` that matches the given names like [Self::find_by_name]. + /// Only attributes with name as [JsxName] can be returned. /// - /// Each name of "names_to_lookup" should be unique. + /// Each name of "names_to_lookup" should be unique. /// - /// Supports maximum of 16 names to avoid stack overflow. Eeach attribute will consume: + /// Supports maximum of 16 names to avoid stack overflow. Each attribute will consume: /// /// - 8 bytes for the [Option] result; /// - plus 16 bytes for the [&str] argument. - pub fn find_attributes_by_name( + pub fn find_by_names( &self, names_to_lookup: [&str; N], ) -> [Option; N] { @@ -257,7 +259,7 @@ impl JsxAttributeList { let mut missing = N; - 'attributes: for att in self.iter() { + 'attributes: for att in self { if let Some(attribute) = att.as_jsx_attribute() { if let Some(name) = attribute .name() @@ -282,40 +284,34 @@ impl JsxAttributeList { results } -} -pub fn find_attribute_by_name( - attributes: JsxAttributeList, - name_to_lookup: &str, -) -> SyntaxResult> { - let attribute = attributes.iter().find_map(|attribute| { - let attribute = JsxAttribute::cast_ref(attribute.syntax())?; - let name = attribute.name().ok()?; - let name = JsxName::cast_ref(name.syntax())?; - if name.value_token().ok()?.text_trimmed() == name_to_lookup { - Some(attribute) - } else { - None - } - }); + pub fn find_by_name(&self, name_to_lookup: &str) -> SyntaxResult> { + let attribute = self.iter().find_map(|attribute| { + let attribute = JsxAttribute::cast_ref(attribute.syntax())?; + let name = attribute.name().ok()?; + let name = JsxName::cast_ref(name.syntax())?; + if name.value_token().ok()?.text_trimmed() == name_to_lookup { + Some(attribute) + } else { + None + } + }); - Ok(attribute) -} + Ok(attribute) + } -pub fn has_trailing_spread_prop( - attributes: JsxAttributeList, - current_attribute: impl Into, -) -> bool { - let mut current_attribute_found = false; - let current_attribute = current_attribute.into(); - for attribute in attributes { - if attribute == current_attribute { - current_attribute_found = true; - continue; - } - if current_attribute_found && attribute.as_jsx_spread_attribute().is_some() { - return true; + pub fn has_trailing_spread_prop(&self, current_attribute: impl Into) -> bool { + let mut current_attribute_found = false; + let current_attribute = current_attribute.into(); + for attribute in self { + if attribute == current_attribute { + current_attribute_found = true; + continue; + } + if current_attribute_found && attribute.as_jsx_spread_attribute().is_some() { + return true; + } } + false } - false } diff --git a/website/docs/src/lint/rules/index.md b/website/docs/src/lint/rules/index.md index 89b262267bc..92f84ab05ca 100644 --- a/website/docs/src/lint/rules/index.md +++ b/website/docs/src/lint/rules/index.md @@ -57,7 +57,7 @@ Enforces the usage of the attribute type for the element butt useKeyWithClickEvents recommended -Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the noKeyPress keyboard event. +Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.

diff --git a/website/docs/src/lint/rules/useKeyWithClickEvents.md b/website/docs/src/lint/rules/useKeyWithClickEvents.md index d6ce6fcd792..9b8c6206677 100644 --- a/website/docs/src/lint/rules/useKeyWithClickEvents.md +++ b/website/docs/src/lint/rules/useKeyWithClickEvents.md @@ -6,7 +6,7 @@ title: Lint Rule useKeyWithClickEvents > This rule is recommended by Rome. -Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `noKeyPress` keyboard event. +Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `onKeyPress` keyboard event. ## Examples @@ -71,3 +71,7 @@ Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, o
{}} >
``` +```jsx + +``` +