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

feat(eslint-plugin-template): [prefer-ngsrc] add rule #1477

Merged
merged 1 commit into from Sep 17, 2023

Conversation

strawberry-choco
Copy link
Contributor

Adds a rule to use the ngSrc attribute over src.

Closes #1265

Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @strawberry-choco!

Just a couple of instances of the same point and then this is good to go

packages/eslint-plugin-template/src/rules/prefer-ngsrc.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-template/src/rules/prefer-ngsrc.ts Outdated Show resolved Hide resolved
@JamesHenry
Copy link
Member

@strawberry-choco You just need to fix up the formatting, you can run yarn format

@strawberry-choco
Copy link
Contributor Author

So, can someone run the CI again?

description: 'should fail image when using both src and ngsrc',
annotatedSource: `
<ng-template>
<img ngSrc="http://localhost" src="http://localhost">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read from the source that the error will always be at the src-attribute, and not necessarily the second attribute.
Might be a good idea to add a test where src and ngSrc are reversed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks @jerone!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@nx-cloud
Copy link

nx-cloud bot commented Sep 3, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4e96532. 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.

@@ -10,6 +10,7 @@
"@angular-eslint/template/mouse-events-have-key-events": "error",
"@angular-eslint/template/no-autofocus": "error",
"@angular-eslint/template/no-distracting-elements": "error",
"@angular-eslint/template/prefer-ngsrc": "error",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this again, it is what is causing the integration tests to fail

A) you should not manually patch this file, it is generated based on using a prefix in the rule description
B) we will not be modifying the accessibility config in minor versions. We will only do that in major versions
C) I don't believe this even is an accessibility rule? It's a web performance rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@JamesHenry
Copy link
Member

@strawberry-choco just following up on this one, please could you add a test case per jerone's comment, and revert the update to the accessibility config?

@strawberry-choco
Copy link
Contributor Author

@strawberry-choco just following up on this one, please could you add a test case per jerone's comment, and revert the update to the accessibility config?

Sorry, got a little busy recently and forgot about this PR. Thanks for the reminder.

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Merging #1477 (4e96532) into main (b1d07bc) will increase coverage by 0.08%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
+ Coverage   89.46%   89.54%   +0.08%     
==========================================
  Files         164      166       +2     
  Lines        3085     3110      +25     
  Branches      525      527       +2     
==========================================
+ Hits         2760     2785      +25     
  Misses        195      195              
  Partials      130      130              
Flag Coverage Δ
unittest 89.54% <100.00%> (+0.08%) ⬆️

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

Files Changed Coverage Δ
packages/eslint-plugin-template/src/index.ts 69.81% <ø> (+0.58%) ⬆️
...s/eslint-plugin-template/src/rules/prefer-ngsrc.ts 100.00% <100.00%> (ø)
...-plugin-template/tests/rules/prefer-ngsrc/cases.ts 100.00% <100.00%> (ø)

@JamesHenry JamesHenry merged commit 0cfbc80 into angular-eslint:main Sep 17, 2023
14 checks passed
@JamesHenry
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

New rule suggestion: no-restricted-attributes
3 participants