feat(eslint-plugin-template): [no-restricted-syntax] add rule #698
Conversation
|
|
||
| ## Rule Options | ||
|
|
||
| The rule does not have any configuration options. |
There was a problem hiding this comment.
Nx Cloud ReportCI ran the following commands for commit 319c778. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch Sent with 💌 from NxCloud. |
|
|
||
| <br> | ||
|
|
||
| ✅ - Examples of **correct** code for this rule: |
There was a problem hiding this comment.
Hmmm .. the generator only grab tests that don't contain options, right? What can we do for this case here since the rule has no defaults + the options are not optional?
Just noticed that we have the same problems for component-selector and directive-selector.
|
|
||
| <br> | ||
|
|
||
| ❌ - Examples of **incorrect** code for this rule: |
There was a problem hiding this comment.
For these examples, it would be great to have the options defined somewhere (perhaps in the form of a comment).
| import type { AbsoluteSourceSpan } from '@angular/compiler'; | ||
| import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
|
|
||
| export function getLocFromAbsoluteSourceSpan( |
There was a problem hiding this comment.
Not sure if it's the best name + the best place, but I do think it's a good ideia to have this utility (we can refactor the rules to use it in another PR).
ba40b99 to
00bcb30
Compare
| // If the `selector` is already computed (it can happens in case of passing `string` + object format), we just ignore it. | ||
| ...(!(selector in selectors) && { [selector]: visitor(message) }), |
There was a problem hiding this comment.
To be clear, it's preventing the following case:
{
"@angular-eslint/template/no-restricted-syntax": [
"error",
"BoundAttribute",
{
"selector": "BoundAttribute" // note that it's duplicated.
},
]
}AFAIK, we can't prevent this using schema.
That being said:
- Not sure if this comment is clear enough, let me know in case of rephrase needed.
- Or perhaps would it be better to give a warning?
There was a problem hiding this comment.
This seems to be the opposite behavior to the existing core rule for JS: https://github.com/eslint/eslint/blob/d36f12f71b3e4f9e9552f1054d7a75be4dc03671/lib/rules/no-restricted-syntax.js#L58
The latest definition for that selector will replace previous ones via the Object.assign.
I think it would be better to match the existing rule here, regardless of whether we agree with it or not
467efc8 to
d8b7a59
Compare
|
Sorry, this one will need to be updated as I don't think it's best to move ahead with the TS 4.4 one yet |
|
Config/options in docs is now supported as of: #699 |
d8b7a59 to
3e9ac6c
Compare
Codecov Report
@@ Coverage Diff @@
## master #698 +/- ##
==========================================
+ Coverage 85.37% 85.55% +0.17%
==========================================
Files 83 85 +2
Lines 1922 1945 +23
Branches 339 343 +4
==========================================
+ Hits 1641 1664 +23
Misses 170 170
Partials 111 111
|
|
Hi, is this waiting for this to be fixed? |
There was a problem hiding this comment.
This is excellent, thanks so much @rafaelss95! Just the one point to deal with around multiple selector definitions.
I'm really sorry it took me so long to get this reviewed, but thank you again for all your hard work 🙏
|
I emailed Rafael about 2 months ago asking if wished to continue working on this but sadly have not received a response in that time. I will therefore close this so that it is clear that it is not being worked on and that somebody else can pick this up if it is important to them. Many thanks to Rafael for his fantastic contributions to angular-eslint to date 🙏 |
Closes #655.
Note that the first two commits are from #697.