always set markers for spaced-comment rule to support reference types #318
Conversation
Example: ```ts /// <reference types="path-to-file" /> ``` fixing for issue typescript-eslint#317
There was a problem hiding this comment.
Thanks so much for sending in this PR @beenotung! There are a few small things that'll need to be addressed. I can take a deeper look at the rule configuration logic then.
| @@ -18,6 +18,13 @@ module.exports = { | |||
| }, | |||
| plugins: ["@typescript-eslint"], | |||
| rules: { | |||
| "@typescript-eslint/array-type": [ | |||
There was a problem hiding this comment.
🤔If you're interested in enabling another rule, would you mind filing a separate issue/PR? I'm generally in favor of this change but it'd be good to keep it separate.
| @@ -4,8 +4,10 @@ | |||
| *.map | |||
| coverage/ | |||
| node_modules/ | |||
| pnpm-lock.yaml | |||
There was a problem hiding this comment.
Same with these: +1 to including this in a separate issue and/or PR?
Guessing you're a happy pnpm user? 😄
There was a problem hiding this comment.
yes, shall we put this and array-type settings separately?
| const capitalizedNotices: string[] = []; | ||
|
|
||
| if (!tslintRule.ruleArguments.includes("check-space")) { | ||
| spaceCommentRuleArguments.push("never"); | ||
| if (!spaceCommentRuleArguments.includes("always")) { |
There was a problem hiding this comment.
Hmm, looks like you're missing unit test coverage for this line per the test coverage report. You'll need to add test coverage for this case to get it to pass.
There was a problem hiding this comment.
I get your point, will made another update
There was a problem hiding this comment.
But I do not know how. I tried to duplicate the test case on line 19, added another version without the check-space in ruleArguments array, but npm run test:ci still complain missing coverage on that line
There was a problem hiding this comment.
The problem on line 15 is that spaceCommentRuleArguments is an empty array so the test suite is not satisfying the condition. Also, shouldn't it be tslintRule.ruleArguments as before?
JoshuaKGoldberg
added
the
status: waiting for author
|
👋 hey again @beenotung! Are you still interested in working on this? If you've run out of time, I can continue working on this PR if that's helpful. |
|
Mmhh, closing this PR to keep the PR queue small. I'll send an equivalent PR to finish up the changes. Thanks for getting this started @beenotung! 💖 |
PR Checklist
status: accepting prsOverview
Always set markers for spaced-comment rule to support reference types.
Example:
/// <reference types="path-to-file" />