[Feature Request] comment-format - no check-space on special patterns/keywords #3951
Comments
@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. |
Ok, but I do not know if I have the skill |
@Shinigami92 you'll want to start with the |
I think I can solve the problem in two different paths First[true, "check-space", {"no-space-keywords": ["TODO", "FIXME"]}] Pros:
Cons:
Second[true, "check-space", "check-lowercase", {"ignore-words": ["TODO", "FIXME"], "ignore-leading-space": true}] Pros:
Cons:
Which solution should I prefer to implement? |
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 |
If there were any breaking changes, I would not like this feature either. |
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. |
I'm already working on this feature That was 12 days ago and I can not invest much time, so it may take a while |
@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 👍 |
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 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 |
@Shinigami92 your index math is off. you'll have to adjust your fix positions to match your expected test |
I looked again at the other tests and saw that it is there exactly the same way. 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 I have updated my branch to the latest Master 5.11.0 So I'm going to remove the [WIP] now and I think it's ready for review and merge |
Hmm, looks like the changes in this PR are actually already resolved by the Please comment here or in #4003 if you think that's a mistake and you'd like this issue to be re-opened! |
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. 😢 |
Bug Report
TypeScript code being linted
with
tslint.json
configuration:Actual behavior
see TypeScript code being linted
Expected behavior
see TypeScript code being linted
The text was updated successfully, but these errors were encountered: