fix(eslint-plugin): [component-selector] enhance check for prefix and… #1250

abaran30 · 2022-12-07T17:59:20Z

...kebab-case

Resolves #318

This pull request contains the following changes to accommodate the discussion had in #318:

Add new error message in rule component-selector for invalid prefix and case
Add test cases to verify the examples posted in the issue

nx-cloud · 2022-12-07T17:59:23Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 74dbacd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 7 targets

Sent with 💌 from NxCloud.

codecov · 2022-12-07T18:07:05Z

Codecov Report

Merging #1250 (74dbacd) into main (023c6c5) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1250      +/-   ##
==========================================
+ Coverage   88.11%   88.18%   +0.07%     
==========================================
  Files         164      164              
  Lines        3172     3175       +3     
  Branches      512      513       +1     
==========================================
+ Hits         2795     2800       +5     
+ Misses        261      260       -1     
+ Partials      116      115       -1

Flag	Coverage Δ
unittest	`88.18% <100.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ages/eslint-plugin/src/rules/component-selector.ts	`94.44% <ø> (+6.20%)`	⬆️
...int-plugin/tests/rules/component-selector/cases.ts	`100.00% <100.00%> (ø)`

abaran30 · 2022-12-07T21:17:14Z

TODO: this change allows for selectors like sgggggggg-foo-bar; not ideal... Thank you @sandikbarr for catching this!

sandikbarr · 2022-12-07T21:25:09Z

packages/eslint-plugin/src/rules/pipe-prefix.ts

@@ -88,7 +88,7 @@ export default createESLintRule<Options, MessageIds>({
          return;
        }

-        if (!prefixValidator.apply(this, [nameValue])) {
+        if (!prefixValidator.apply(this, [nameValue]) || !styleValidator) {


Does this styleValidator need to be called as a function with the nameValue?

You're right, it does. However, I'm currently looking to see if this change can be reverted. I better understand the importance of the suffix logic in prefix, and might look to bring it back.

abaran30 · 2022-12-07T23:07:59Z

packages/eslint-plugin/tests/rules/component-selector/cases.ts

@@ -302,9 +312,9 @@ export const invalid = [
        })
        class Test {}
      `,
-    messageId: messageIdStyleFailure,
+    messageId: messageIdStyleAndPrefixFailure,


Note: the reported error for this invalid case has changed to the new error message. Although it's not incorrect, the former is more concise for this particular case. I think the only way to retain the original message is to move the style logic inside the prefix validator to its own validator (which I did attempt in a previous commit). However, this will result in a more complex conditional in the rule when determining which message to report.

LMK your thoughts!

This seems acceptable to me as a non breaking change while addressing the original issue.

sandikbarr

thanks for digging into this!

sandikbarr · 2022-12-07T23:15:15Z

packages/eslint-plugin/tests/rules/component-selector/cases.ts

@@ -302,9 +312,9 @@ export const invalid = [
        })
        class Test {}
      `,
-    messageId: messageIdStyleFailure,
+    messageId: messageIdStyleAndPrefixFailure,


This seems acceptable to me as a non breaking change while addressing the original issue.

abaran30 · 2023-01-10T16:53:17Z

@JamesHenry ready for review. LMK what you think.

… kebab-case

… readability

… and checks

JamesHenry

Thank you @abaran30 (and @sandikbarr)! And sorry for the delay in reviewing

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular-eslint/builder](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`15.1.0` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2fbuilder/15.1.0/15.2.0) | | [@angular-eslint/eslint-plugin](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`15.1.0` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2feslint-plugin/15.1.0/15.2.0) | | [@angular-eslint/eslint-plugin-template](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`15.1.0` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2feslint-plugin-template/15.1.0/15.2.0) | | [@angular-eslint/schematics](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`15.1.0` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2fschematics/15.1.0/15.2.0) | | [@angular-eslint/template-parser](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`15.1.0` -> `15.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2ftemplate-parser/15.1.0/15.2.0) | --- ### Release Notes <details> <summary>angular-eslint/angular-eslint (@angular-eslint/builder)</summary> ### [`v15.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/builder/CHANGELOG.md#1520-httpsgithubcomangular-eslintangular-eslintcomparev1510v1520-2023-01-14) [Compare Source](angular-eslint/angular-eslint@v15.1.0...v15.2.0) **Note:** Version bump only for package [@angular-eslint/builder](https://github.com/angular-eslint/builder) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/eslint-plugin)</summary> ### [`v15.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#1520-httpsgithubcomangular-eslintangular-eslintcomparev1510v1520-2023-01-14) [Compare Source](angular-eslint/angular-eslint@v15.1.0...v15.2.0) ##### Bug Fixes - **eslint-plugin:** \[component-selector] enhance check for prefix and kebab-case ([#1250](angular-eslint/angular-eslint#1250)) ([16827e4](angular-eslint/angular-eslint@16827e4)) - **eslint-plugin:** \[no-inputs-metadata-property] do not report on directive composition API ([#1248](angular-eslint/angular-eslint#1248)) ([539cf9f](angular-eslint/angular-eslint@539cf9f)) - update typescript-eslint packages to v5.45.1 ([#1239](angular-eslint/angular-eslint#1239)) ([abb7f79](angular-eslint/angular-eslint@abb7f79)) - update typescript-eslint packages to v5.48.1 ([#1255](angular-eslint/angular-eslint#1255)) ([11151d1](angular-eslint/angular-eslint@11151d1)) ##### Features - **eslint-plugin:** \[require-localize-metadata] option to require meaning ([#1235](angular-eslint/angular-eslint#1235)) ([b870123](angular-eslint/angular-eslint@b870123)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/eslint-plugin-template)</summary> ### [`v15.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/eslint-plugin-template/CHANGELOG.md#1520-httpsgithubcomangular-eslintangular-eslintcomparev1510v1520-2023-01-14) [Compare Source](angular-eslint/angular-eslint@v15.1.0...v15.2.0) ##### Bug Fixes - update typescript-eslint packages to v5.45.1 ([#1239](angular-eslint/angular-eslint#1239)) ([abb7f79](angular-eslint/angular-eslint@abb7f79)) - update typescript-eslint packages to v5.48.1 ([#1255](angular-eslint/angular-eslint#1255)) ([11151d1](angular-eslint/angular-eslint@11151d1)) ##### Features - **eslint-plugin-template:** \[i18n] option to require i18n metadata meaning ([#1234](angular-eslint/angular-eslint#1234)) ([4ef0290](angular-eslint/angular-eslint@4ef0290)) - **eslint-plugin-template:** \[no-interpolation-in-attributes] new rule added ([#1242](angular-eslint/angular-eslint#1242)) ([977cb3a](angular-eslint/angular-eslint@977cb3a)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/schematics)</summary> ### [`v15.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/schematics/CHANGELOG.md#1520-httpsgithubcomangular-eslintangular-eslintcomparev1510v1520-2023-01-14) [Compare Source](angular-eslint/angular-eslint@v15.1.0...v15.2.0) ##### Bug Fixes - update dependency eslint to v8.29.0 ([#1246](angular-eslint/angular-eslint#1246)) ([10c14d2](angular-eslint/angular-eslint@10c14d2)) - update dependency eslint to v8.31.0 ([#1262](angular-eslint/angular-eslint#1262)) ([db89c85](angular-eslint/angular-eslint@db89c85)) - update dependency ignore to v5.2.1 ([#1237](angular-eslint/angular-eslint#1237)) ([609e06b](angular-eslint/angular-eslint@609e06b)) - update dependency ignore to v5.2.4 ([#1263](angular-eslint/angular-eslint#1263)) ([a220e0c](angular-eslint/angular-eslint@a220e0c)) - update typescript-eslint packages to v5.45.1 ([#1239](angular-eslint/angular-eslint#1239)) ([abb7f79](angular-eslint/angular-eslint@abb7f79)) - update typescript-eslint packages to v5.48.1 ([#1255](angular-eslint/angular-eslint#1255)) ([11151d1](angular-eslint/angular-eslint@11151d1)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/template-parser)</summary> ### [`v15.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/template-parser/CHANGELOG.md#1520-httpsgithubcomangular-eslintangular-eslintcomparev1510v1520-2023-01-14) [Compare Source](angular-eslint/angular-eslint@v15.1.0...v15.2.0) ##### Features - **eslint-plugin-template:** \[no-interpolation-in-attributes] new rule added ([#1242](angular-eslint/angular-eslint#1242)) ([977cb3a](angular-eslint/angular-eslint@977cb3a)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1733 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

abaran30 marked this pull request as draft December 7, 2022 21:24

sandikbarr reviewed Dec 7, 2022

View reviewed changes

abaran30 commented Dec 7, 2022

View reviewed changes

abaran30 marked this pull request as ready for review December 7, 2022 23:08

sandikbarr previously approved these changes Dec 7, 2022

View reviewed changes

abaran30 dismissed sandikbarr’s stale review via 56b0670 January 10, 2023 16:52

abaran30 force-pushed the abaran30/fix/component-selectors-single-word-kebab-case branch from 4b4b290 to 56b0670 Compare January 10, 2023 16:52

abaran30 added 4 commits January 14, 2023 18:28

fix(eslint-plugin): [component-selector] enhance check for prefix and…

94245e1

… kebab-case

chore(eslint-plugin): [component-selector] update rule docs

41af701

refactor(eslint-plugin): [component-selector] switch logic around for…

63a3922

… readability

refactor(eslint-plugin): [component-selector] simplify implementation…

74dbacd

… and checks

JamesHenry force-pushed the abaran30/fix/component-selectors-single-word-kebab-case branch from 56b0670 to 74dbacd Compare January 14, 2023 14:28

JamesHenry approved these changes Jan 14, 2023

View reviewed changes

JamesHenry merged commit 16827e4 into angular-eslint:main Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [component-selector] enhance check for prefix and… #1250

fix(eslint-plugin): [component-selector] enhance check for prefix and… #1250

abaran30 commented Dec 7, 2022 •

edited

nx-cloud bot commented Dec 7, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited

abaran30 commented Dec 7, 2022

sandikbarr Dec 7, 2022

abaran30 Dec 7, 2022

abaran30 Dec 7, 2022

sandikbarr Dec 7, 2022

sandikbarr left a comment

sandikbarr Dec 7, 2022

abaran30 commented Jan 10, 2023

JamesHenry left a comment

fix(eslint-plugin): [component-selector] enhance check for prefix and… #1250

fix(eslint-plugin): [component-selector] enhance check for prefix and… #1250

Conversation

abaran30 commented Dec 7, 2022 • edited

nx-cloud bot commented Dec 7, 2022 • edited

☁️ Nx Cloud Report

codecov bot commented Dec 7, 2022 • edited

Codecov Report

abaran30 commented Dec 7, 2022

sandikbarr Dec 7, 2022

Choose a reason for hiding this comment

abaran30 Dec 7, 2022

Choose a reason for hiding this comment

abaran30 Dec 7, 2022

Choose a reason for hiding this comment

sandikbarr Dec 7, 2022

Choose a reason for hiding this comment

sandikbarr left a comment

Choose a reason for hiding this comment

sandikbarr Dec 7, 2022

Choose a reason for hiding this comment

abaran30 commented Jan 10, 2023

JamesHenry left a comment

Choose a reason for hiding this comment

abaran30 commented Dec 7, 2022 •

edited

nx-cloud bot commented Dec 7, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited