feat(eslint-plugin-template): [self-closing-tags] add rule #1322
Conversation
☁️ Nx Cloud ReportAttention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here. CI is running/has finished running commands for commit c39ce93. 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. |
aleesaan
force-pushed
the
self-closing-tags
branch
2 times, most recently
from
3d1fada
to
45b1d9b
Compare
There was a problem hiding this comment.
Thanks a lot for this @aleesaan! Please see the comments inline
There was a problem hiding this comment.
Please use the existing getDomElements() utility that we have and do not introduce a new list for us to maintain separately.
You can have a look in https://github.com/angular-eslint/angular-eslint/blob/6abcc3ab0d3000d00ab668d2f6776e0f851b0a41/packages/eslint-plugin-template/src/rules/accessibility-role-has-required-aria.ts for precedent on how to select native elements (and here you want to do the inverse, and only select non-native)
There was a problem hiding this comment.
Thank you so much for the feedback! I was in fact looking for this kind of suggestions 😅 I pushed the fixes, let me know how it looks.
packages/eslint-plugin-template/tests/rules/self-closing-tags/cases.ts
Outdated
Show resolved
Hide resolved
| const parserServices = getTemplateParserServices(context); | ||
|
|
||
| return { | ||
| Element$1({ |
There was a problem hiding this comment.
Please see comment about applying the filter of non-native elements in the selector
packages/eslint-plugin-template/docs/rules/self-closing-tags.md
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/prefer-self-closing-tags.ts
Outdated
Show resolved
Hide resolved
| (startSourceSpan.start.offset === endSourceSpan.start.offset && | ||
| startSourceSpan.end.offset === endSourceSpan.end.offset); | ||
|
|
||
| if (isNative || !noContent || noCloseTag) { |
There was a problem hiding this comment.
We should exit early on isNative as soon as we know about it on line 40
|
Thanks a lot @aleesaan after that one final change and syncing up with the latest this is ready to merge! |
aleesaan
force-pushed
the
self-closing-tags
branch
from
e4b0ac2
to
7811d35
Compare
|
Fixed the last things and rebased! Let me know if it's all looking good :) |
aleesaan
force-pushed
the
self-closing-tags
branch
from
9646729
to
c39ce93
Compare
Codecov Report
@@ Coverage Diff @@
## main #1322 +/- ##
==========================================
+ Coverage 89.30% 89.39% +0.09%
==========================================
Files 160 162 +2
Lines 2991 3018 +27
Branches 501 506 +5
==========================================
+ Hits 2671 2698 +27
Misses 200 200
Partials 120 120
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Implements the feature proposed in this discussion.
Let me know If there's a better way to check that an element has no content, and thanks for the awesome plugin!