Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eslint-plugin): export require-localize-metadata rule #1469

Merged

Conversation

amikheychik
Copy link
Contributor

Prevents the Definition for rule '@angular-eslint/require-localize-metadata' was not found ESLint error.

Also, fix sorted order for the prefer-standalone-component rule.

amikheychik added a commit to perfective/eslint-config-angular that referenced this pull request Aug 5, 2023
While there are no Angular files in the repository, it helps to prevent the most trivial bugs for now.

- Temporarily disable the `@angular-eslint/require-localize-metadata` rule (see angular-eslint/angular-eslint#1469).
- Fix `@angular-eslint/template/prefer-self-closing-tags` config.
@abaran30
Copy link
Contributor

abaran30 commented Aug 6, 2023

@amikheychik thanks very much for catching this!

It looks like something went wrong with the code formatting check (see here). Would you be able to take a look?

FYI @JamesHenry let's also add this rule to the all config when appropriate.

@amikheychik amikheychik force-pushed the fix/require-localize-metadata branch 2 times, most recently from 2e421d9 to 8cb29bc Compare August 7, 2023 01:33
@amikheychik
Copy link
Contributor Author

@abaran30 I've updated the formatting, should be good now.

I've also added the rule to the configs/all.json, as it should contain all existing rules.

Prevents the `Definition for rule '@angular-eslint/require-localize-metadata' was not found` ESLint error.

Also, fix sorted order for the `prefer-standalone-component` rule.
@amikheychik amikheychik force-pushed the fix/require-localize-metadata branch from 8cb29bc to 492eb23 Compare August 7, 2023 01:36
@abaran30 abaran30 self-requested a review August 7, 2023 01:36
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1469 (492eb23) into main (7d1f592) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1469   +/-   ##
=======================================
  Coverage   89.32%   89.32%           
=======================================
  Files         162      162           
  Lines        3044     3045    +1     
  Branches      515      515           
=======================================
+ Hits         2719     2720    +1     
  Misses        201      201           
  Partials      124      124           
Flag Coverage Δ
unittest 89.32% <ø> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
packages/eslint-plugin/src/index.ts 69.64% <ø> (+0.55%) ⬆️

@abaran30
Copy link
Contributor

abaran30 commented Aug 7, 2023

Thank you @amikheychik!

I'm going to check and make sure that it'd be okay to update the all config in this PR. We wouldn't want to surprise users of the all config unexpectedly with this rule on the next release.

@amikheychik
Copy link
Contributor Author

@abaran30 the problem is that without this rule in the all.json tests fail (configs › all › should contain all of the rules from the plugin)

BTW, this bug is present since v13.2.0 when the rule was introduced. Would you like me to backport it to v13, v14, and v15?

@JamesHenry
Copy link
Member

Yeah new rules should always be added to the all config, it’s expected and can’t wait until major releases. I think users wanting to enable all rules all the time is a pretty strange idea personally but the option is there for those that do

@abaran30 abaran30 merged commit e3dc936 into angular-eslint:main Aug 7, 2023
13 checks passed
@amikheychik
Copy link
Contributor Author

@JamesHenry would you publish v15, v14, and v13 patch versions if I backport this patch there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants