Tslint: [Feature Request] comment-format - no check-space on special patterns/keywords

[Shinigami92]

Bug Report

• __TSLint version__: 5.10.0
• __TypeScript version__: 2.9.1
• __Running TSLint via__: CLI / VSCode

TypeScript code being linted

// comment ok
// Comment maybe ok
//comment NOT ok
//Comment NOT ok
//TODO: comment ok cause TODO: is a registered pattern

with tslint.json configuration:

{
    "comment-format": [true, "check-space", {"ignore-words": ["TODO", "FIXME"]}]
}

Actual behavior

see _TypeScript code being linted_

Expected behavior

see _TypeScript code being linted_

[giladgray]

@Shinigami92 if you're particularly passionate about this feature, please submit a PR yourself. otherwise, close this issue. i see little value in this special casing.

[Shinigami92]

Ok, but I do not know if I have the skill
could take a bit until I found myself into the code
Can you give me a hint where I could start?

[giladgray]

@Shinigami92 you'll want to start with the comment-format rule and the Develop section on the docs website.

[Shinigami92]

I think I can solve the problem in two different paths

First

[true, "check-space", {"no-space-keywords": ["TODO", "FIXME"]}]

Pros:

• keywords can be explicitly configured
• can be used without "check-lowercase" or "check-uppercase"

Cons:

• new option element in the rule

Second

[true, "check-space", "check-lowercase", {"ignore-words": ["TODO", "FIXME"], "ignore-leading-space": true}]

Pros:

• no new option element
• merges with already existing option element
• works together with "ignore-words" or "ignore-pattern"

Cons:

• only useful if true is set
• works only if check-space is given

---

Which solution should I prefer to implement?

[giladgray]

I prefer the first option because it is much simpler. But again, i'm not really passionate about this feature at all and would prefer to just put that space after the // _always_ or _never_ but not special case certain keywords.

[Shinigami92]

If there were any breaking changes, I would not like this feature either.
But as long as everything is optional, it can only enhance the user experience.
When I have more time I will also be happy to take care of further documentation improvements for tslint

[giladgray]

yeah it's not breaking but with a project like this there's a fine line between useful and burdensome.

i will not implement this feature because i, as a tslint maintainer, find it non-standard and specific to your workflow. but if you really want it and submit a PR, i'll happily review it.

[Shinigami92]

I'm already working on this feature
Current work can be seen here: https://github.com/Shinigami92/tslint/compare/master...Shinigami92:feature/comment-format-check-space-special-keywords

That was 12 days ago and I can not invest much time, so it may take a while
But as you've noticed, this feature is not so important that it needs to be there in the near future :)
I also see it as an introduction to learning AST for me

[giladgray]

@Shinigami92 sweet. yes a contribution like this is ideal for learning, and just you wait for the code review! we'll be here when you're ready 👍

[Shinigami92]

So the first cool thing is: it works \o/

I prefixed the PR with [WIP] because I think you can review it but I should add documentation to this
Or should the documentation part in an extra PR?

---

Also I'm a bit confused about the test behaviour when it checks line 35

should be:

//TODO: Todo description
        ~~~~~~~~~~~~~~~~ [lower]
       ↑

but needs to be:

//TODO: Todo description
       ~~~~~~~~~~~~~~~~~ [lower]
       ↑

otherwise I have to break other old tests

---

Also the NO_SPACE_KEYWORDS_FACTORY is not in use currently

[giladgray]

@Shinigami92 your index math is off. you'll have to adjust your fix positions to match your expected test ~~~s

[Shinigami92]

I looked again at the other tests and saw that it is there exactly the same way.
Like here: lower/test.ts.lint#L5-L6

Therefore, I assume that my approach was the wrong one and that it is correct that it needs to be

//TODO: Todo description
       ~~~~~~~~~~~~~~~~~ [lower]
       ↑

Thus, if all tests are wrong in the comment-format in this behavior, this should be a dedicated PR
Apart from the fact that it is only a warning underscore more here ^^

---

I have updated my branch to the latest Master 5.11.0
I removed the unused NO_SPACE_KEYWORDS_FACTORY
I'll add codeExamples in a dedicated PR after it's merged

So I'm going to remove the [WIP] now and I think it's ready for review and merge

[JoshuaKGoldberg]

Hmm, looks like the changes in this PR are actually already resolved by the ignore-pattern option in #3583. There are some pretty substantial merge conflicts in the associated PR 4003 that are otherwise nontrivial to resolve as well. Closing this PR for housekeeping.

Please comment here or in #4003 if you think that's a mistake and you'd like this issue to be re-opened!

[JoshuaKGoldberg]

By the way, @Shinigami92 looking back through this, I think you did a great job of explaining the use case, technical tradeoffs associated with the implementation, and decisions made around how to best deal with it. It feels bad closing an issue with such great discussion. 😢