feat(eslint-plugin-template): add require-localize-metadata rule #844

abaran30 · 2021-11-23T19:08:13Z

Similar to #804, this pull request adds the option to enforce the requirement of a description for i18n metadata when using $localize in component code.

TODO:

Address all questions/comments below
Documentation

Pull request is in draft mode; please see my comments below!

FYI @JamesHenry , I know that you haven't used Angular i18n, but perhaps you could chime in on the creation of this new rule? See my comments for require-localize-metadata.ts.

nx-cloud · 2021-11-23T19:10:43Z

☁️ Nx Cloud Report

CI ran the following commands for commit 10f48b8. 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.

abaran30 · 2021-11-23T19:16:33Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+
+const VALID_LOCALIZED_STRING_WITH_DESCRIPTION = new RegExp(/`:[^|]*:.*`/);
+
+// TODO: The following values are also used in eslint-plugin-template/i18n, and should likely be centralized


What is a good location for these values?

I would remove the comment, I think having the small bit of repetition is fine and allows for them to remain independent and be more easily refactored if the structure of Angular guides changes in future

packages/eslint-plugin/src/rules/require-localize-metadata.ts

abaran30 · 2021-11-23T19:19:54Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+  requireDescription: false,
+};
+
+const VALID_LOCALIZED_STRING_WITH_DESCRIPTION = new RegExp(/`:[^|]*:.*`/);


Any thoughts on this RegExp?

packages/eslint-plugin/tests/rules/require-localize-metadata/cases.ts

aokrushk · 2022-02-11T19:39:09Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+        taggedTemplateExpression: TSESTree.TaggedTemplateExpression,
+      ) {
+        const identifierName = (
+          taggedTemplateExpression.tag as TSESTree.Identifier


why does it need casting?

@adbaran1 It would be better to narrow the type through control flow analysis by actually checking the shape of the tag to check that it is an identifier.

You can use the utility TSESLintASTUtils.isIdentifier(...) from @typescript-eslint/experimental-utils to do exactly what you need and TypeScript will pick up on the types through the control flow analysis.

packages/eslint-plugin/src/rules/require-localize-metadata.ts

JamesHenry

Thanks a lot for this @adbaran1, I'm really sorry for how long it has taken me to get back to you on this.

I moved countries a couple of times so things have been very chaotic.

JamesHenry · 2022-02-13T19:09:59Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+
+const VALID_LOCALIZED_STRING_WITH_DESCRIPTION = new RegExp(/`:[^|]*:.*`/);
+
+// TODO: The following values are also used in eslint-plugin-template/i18n, and should likely be centralized


I would remove the comment, I think having the small bit of repetition is fine and allows for them to remain independent and be more easily refactored if the structure of Angular guides changes in future

JamesHenry · 2022-02-13T19:11:50Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+  },
+  defaultOptions: [DEFAULT_OPTIONS],
+  create(context, [{ requireDescription }]) {
+    function reportRequireLocalizeMetadataError(


Please can you refactor this implementation to just inline everything in the Node handler method?

There is very little code here but the dedication to wrapping every step into functions hurts readability in my opinion because of how it changes the flow of the document.

See my comment below about eventually being able to support custom ID and meaning.

Makes sense @adbaran1 👍

JamesHenry · 2022-02-13T19:16:45Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+        taggedTemplateExpression: TSESTree.TaggedTemplateExpression,
+      ) {
+        const identifierName = (
+          taggedTemplateExpression.tag as TSESTree.Identifier


@adbaran1 It would be better to narrow the type through control flow analysis by actually checking the shape of the tag to check that it is an identifier.

You can use the utility TSESLintASTUtils.isIdentifier(...) from @typescript-eslint/experimental-utils to do exactly what you need and TypeScript will pick up on the types through the control flow analysis.

JamesHenry · 2022-02-13T19:20:04Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+      {
+        type: 'object',
+        properties: {
+          requireDescription: {


I don't understand what purpose this option serves given the whole rule is dedicated to this purpose?

This is further backed up by the fact you don't have tests for when it is disabled.

I would think that if someone does not want to require the description they would simply not use this rule right?

@JamesHenry no worries, and I hope things are less chaotic now 😄 . The reason for this setup is to be able to support custom ID and meaning, which are the other two i18n metadata parameters (see i18n metadata for translation). This pull request is only to support description for the time being, which is why the implementation looks the way it does (i.e. having functions like testLocalizeTemplateElementWithDescription).

With that all said, let me know what you think about the overall approach.

…es contain a description

… behavior

…lateExpression

codecov · 2022-02-22T15:48:58Z

Codecov Report

Merging #844 (10f48b8) into master (4c11177) will increase coverage by 0.09%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   86.36%   86.45%   +0.09%     
==========================================
  Files         145      147       +2     
  Lines        2720     2746      +26     
  Branches      438      441       +3     
==========================================
+ Hits         2349     2374      +25     
  Misses        220      220              
- Partials      151      152       +1

Flag	Coverage Δ
unittest	`86.45% <96.15%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
...lint-plugin/src/rules/require-localize-metadata.ts	`95.00% <95.00%> (ø)`
...gin/tests/rules/require-localize-metadata/cases.ts	`100.00% <100.00%> (ø)`

aokrushk · 2022-02-22T15:39:45Z

packages/eslint-plugin/src/rules/require-localize-metadata.ts

+  },
+  defaultOptions: [DEFAULT_OPTIONS],
+  create(context, [{ requireDescription }]) {
+    function reportRequireLocalizeMetadataError(


Makes sense @adbaran1 👍

alex-okrushko · 2022-03-18T15:11:21Z

@JamesHenry Looks like Adrian has addresses all the comments. Anything else needs to be done before it's merged?
Thanks!

JamesHenry · 2022-04-02T09:51:17Z

Thanks again for your patience on this @adbaran1 @alex-okrushko.

I think the description of the overall rule matching the intent behind a subset of its intended features is still confusing but I will amend that post-merge

abaran30 · 2022-04-04T12:49:59Z

Thank you @JamesHenry. Please let us know how we can assist you in making it all better. We can look to add support for Angular i18n meaning and IDs to make the overall setup more clear ASAP.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular-eslint/builder](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`13.1.0` -> `13.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2fbuilder/13.1.0/13.2.0) | | [@angular-eslint/eslint-plugin](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`13.1.0` -> `13.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2feslint-plugin/13.1.0/13.2.0) | | [@angular-eslint/eslint-plugin-template](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`13.1.0` -> `13.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2feslint-plugin-template/13.1.0/13.2.0) | | [@angular-eslint/schematics](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`13.1.0` -> `13.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2fschematics/13.1.0/13.2.0) | | [@angular-eslint/template-parser](https://github.com/angular-eslint/angular-eslint) | devDependencies | minor | [`13.1.0` -> `13.2.0`](https://renovatebot.com/diffs/npm/@angular-eslint%2ftemplate-parser/13.1.0/13.2.0) | --- ### Release Notes <details> <summary>angular-eslint/angular-eslint (@angular-eslint/builder)</summary> ### [`v13.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/builder/CHANGELOG.md#1320-httpsgithubcomangular-eslintangular-eslintcomparev1310v1320-2022-04-03) [Compare Source](angular-eslint/angular-eslint@v13.1.0...v13.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> ### [`v13.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#1320-httpsgithubcomangular-eslintangular-eslintcomparev1310v1320-2022-04-03) [Compare Source](angular-eslint/angular-eslint@v13.1.0...v13.2.0) ##### Features - **eslint-plugin-template:** add require-localize-metadata rule ([#844](angular-eslint/angular-eslint#844)) ([ca1edf0](angular-eslint/angular-eslint@ca1edf0)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/eslint-plugin-template)</summary> ### [`v13.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/eslint-plugin-template/CHANGELOG.md#1320-httpsgithubcomangular-eslintangular-eslintcomparev1310v1320-2022-04-03) [Compare Source](angular-eslint/angular-eslint@v13.1.0...v13.2.0) ##### Features - **parser:** propagate parse errors from angular compiler ([#969](angular-eslint/angular-eslint#969)) ([ab9b496](angular-eslint/angular-eslint@ab9b496)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/schematics)</summary> ### [`v13.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/schematics/CHANGELOG.md#1320-httpsgithubcomangular-eslintangular-eslintcomparev1310v1320-2022-04-03) [Compare Source](angular-eslint/angular-eslint@v13.1.0...v13.2.0) ##### Bug Fixes - **schematics:** support more permutations of ng-add ([#970](angular-eslint/angular-eslint#970)) ([0bf549b](angular-eslint/angular-eslint@0bf549b)) </details> <details> <summary>angular-eslint/angular-eslint (@angular-eslint/template-parser)</summary> ### [`v13.2.0`](https://github.com/angular-eslint/angular-eslint/blob/HEAD/packages/template-parser/CHANGELOG.md#1320-httpsgithubcomangular-eslintangular-eslintcomparev1310v1320-2022-04-03) [Compare Source](angular-eslint/angular-eslint@v13.1.0...v13.2.0) ##### Features - **parser:** propagate parse errors from angular compiler ([#969](angular-eslint/angular-eslint#969)) ([ab9b496](angular-eslint/angular-eslint@ab9b496)) </details> --- ### Configuration 📅 **Schedule**: 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, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <[email protected]> Co-authored-by: 6543 <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1281 Reviewed-by: 6543 <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>

abaran30 force-pushed the feat/localize-require-metadata branch from f2bde02 to 5b2d478 Compare November 23, 2021 19:10

abaran30 commented Nov 23, 2021

View reviewed changes

packages/eslint-plugin/src/rules/require-localize-metadata.ts Outdated Show resolved Hide resolved

abaran30 commented Nov 23, 2021

View reviewed changes

abaran30 commented Feb 7, 2022

View reviewed changes

packages/eslint-plugin/tests/rules/require-localize-metadata/cases.ts Show resolved Hide resolved

aokrushk reviewed Feb 11, 2022

View reviewed changes

abaran30 marked this pull request as ready for review February 11, 2022 20:28

JamesHenry changed the title ~~feat(require-localize-metadata): option to enforce that $localize tagged messages contain a description~~ feat(eslint-plugin-template): add require-localize-metadata rule Feb 13, 2022

JamesHenry requested changes Feb 13, 2022

View reviewed changes

abaran30 added 8 commits February 14, 2022 09:14

feat(i18n): option to require description for i18n metadata

06203e9

feat(require-localize-metadata): option to enforce that tagged messag…

7b306fb

…es contain a description

refactor(i18n-cases): undo auto format of test file for expected test…

1aff3aa

… behavior

refactor(require-localize-metadata): refactor to check for TaggedTemp…

6486bbf

…lateExpression

test(require-localize-metadata): cases for meaning and custom ID

5b90eb2

refactor(require-localize-feedback): remove unnecessary conditions

7c46a16

docs(require-localize-metadata): generate docs

8f8ae62

refactor(require-localize-metadata): narrow type through control flow

1d4eed3

abaran30 force-pushed the feat/localize-require-metadata branch from 2dfe0a9 to 1d4eed3 Compare February 14, 2022 15:15

fix(localize-require-metadata): import type

c1fb9bf

refactor(require-localize-metadata): simplify implementation

10f48b8

aokrushk approved these changes Feb 22, 2022

View reviewed changes

JamesHenry approved these changes Apr 2, 2022

View reviewed changes

JamesHenry merged commit ca1edf0 into angular-eslint:master Apr 2, 2022

sync-by-unito bot mentioned this pull request May 1, 2022

build(deps-dev): bump @angular-eslint/eslint-plugin-template from 12.3.1 to 13.2.1 numbersprotocol/capture-lite#1582

Closed

sync-by-unito bot mentioned this pull request Jul 1, 2022

build(deps-dev): bump @angular-eslint/eslint-plugin from 12.3.1 to 14.0.0 numbersprotocol/capture-lite#1760

Closed

sync-by-unito bot mentioned this pull request Aug 1, 2022

build(deps-dev): bump @angular-eslint/eslint-plugin from 12.3.1 to 14.0.2 numbersprotocol/capture-lite#1873

Closed

sync-by-unito bot mentioned this pull request Sep 1, 2022

build(deps-dev): bump @angular-eslint/eslint-plugin from 12.3.1 to 14.0.3 numbersprotocol/capture-lite#2006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin-template): add require-localize-metadata rule #844

feat(eslint-plugin-template): add require-localize-metadata rule #844

abaran30 commented Nov 23, 2021 •

edited

Loading

nx-cloud bot commented Nov 23, 2021 •

edited

Loading

abaran30 Nov 23, 2021

JamesHenry Feb 13, 2022

abaran30 Nov 23, 2021

aokrushk Feb 11, 2022

abaran30 Feb 11, 2022

JamesHenry Feb 13, 2022

JamesHenry left a comment

JamesHenry Feb 13, 2022

JamesHenry Feb 13, 2022

abaran30 Feb 14, 2022

aokrushk Feb 22, 2022

JamesHenry Feb 13, 2022

JamesHenry Feb 13, 2022

abaran30 Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 22, 2022 •

edited

Loading

aokrushk Feb 22, 2022

alex-okrushko commented Mar 18, 2022

JamesHenry commented Apr 2, 2022

abaran30 commented Apr 4, 2022


		const VALID_LOCALIZED_STRING_WITH_DESCRIPTION = new RegExp(/`:[^\|]:.`/);

		// TODO: The following values are also used in eslint-plugin-template/i18n, and should likely be centralized

feat(eslint-plugin-template): add require-localize-metadata rule #844

feat(eslint-plugin-template): add require-localize-metadata rule #844

Conversation

abaran30 commented Nov 23, 2021 • edited Loading

nx-cloud bot commented Nov 23, 2021 • edited Loading

☁️ Nx Cloud Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesHenry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abaran30 Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

alex-okrushko commented Mar 18, 2022

JamesHenry commented Apr 2, 2022

abaran30 commented Apr 4, 2022

abaran30 commented Nov 23, 2021 •

edited

Loading

nx-cloud bot commented Nov 23, 2021 •

edited

Loading

abaran30 Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 22, 2022 •

edited

Loading