Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📎 Implement nursery/noTsComment - typescript-eslint/ban-ts-comment #713

Open
unvalley opened this issue Nov 13, 2023 · 16 comments
Open
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@unvalley
Copy link
Member

unvalley commented Nov 13, 2023

Description

Implement typescript-eslint rule ban-ts-comment as noTsComment.
If you have a better name, please suggest it.


Want to contribute? Lets you know you are interested! We will assign you to the issue to prevent several people to work on the same issue. Don't worry, we can unassign you later if you are no longer interested in the issue! Read our contributing guide and analyzer contributing guide.

@unvalley unvalley added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Nov 13, 2023
@ematipico
Copy link
Member

ematipico commented Nov 13, 2023

I am not sure we should add this rule. I am not very fond of it, I'm not sure why we should ban directives. I am happy to change my mind if there's a good argument in favour of this rule.

The only rule that I think makes sense to have is the one to ban @ts-ignore in favour of @ts-expect-error

@unvalley
Copy link
Member Author

@ematipico
If there are only a few members on a given team who are proficient in TypeScript, I think this lint is useful. I have seen a fair amount of code where @ts-ignore is left out, so I see a certain value in this rule.
typescript-eslint offers a flexible option, but it may be better to focus on @ts-ignore.

@ematipico
Copy link
Member

Thank you @unvalley for the argument! I'm convinced :)

@arthurfiorette
Copy link

The only rule that I think makes sense to have is the one to ban @ts-ignore in favour of @ts-expect-error

Banning @ts-ignore in favour of @ts-expect-error is a better alternative overall, also requiring a brief explanation after this directive should also be required: // @ts-expect-error - <explanation>.

However I had to use // @ts-ignore in the past inside environments where a type declaration may or may not be defined, depending if its like inside CI or not, thus only // @ts-ignore would work. Also, I'm not sure if having to enable it to the entire repository just because one place needs it would be a good solution.

@ematipico ematipico added the S-Help-wanted Status: you're familiar with the code base and want to help the project label Nov 14, 2023
@unvalley unvalley self-assigned this Nov 24, 2023
@unvalley
Copy link
Member Author

unvalley commented Nov 27, 2023

@arthurfiorette

However I had to use // @ts-ignore in the past inside environments where a type declaration may or may not be defined, depending if its like inside CI or not, thus only // @ts-ignore would work. Also, I'm not sure if having to enable it to the entire repository just because one place needs it would be a good solution.

How about using biome-ignore for such cases? I think we are not needed to turning off the lint rule in entire repository.

@ematipico
Copy link
Member

Ping @unvalley . Are you still interested?

@unvalley unvalley removed their assignment Jan 4, 2024
@arthurfiorette
Copy link

How about using biome-ignore for such cases? I think we are not needed to turning off the lint rule in entire repository.

Should do the job for such specific use cases.

@Conaclos Conaclos changed the title 📎 Implement lint/noTsComment - typescript-eslint/ban-ts-comment 📎 Implement nursery/noTsComment - typescript-eslint/ban-ts-comment Mar 14, 2024
@emab
Copy link
Contributor

emab commented Apr 21, 2024

Happy to look into this one, however would be good just to verify exactly what this discussion landed on in terms of implementation?

@ematipico
Copy link
Member

Happy to look into this one, however would be good just to verify exactly what this discussion landed on in terms of implementation?

I think, for now, we want to offer a rule that checks @ts-ignore and suggests the expect error directive

@emab
Copy link
Contributor

emab commented Apr 21, 2024

And we'll be using the rule name banTsComment to do so?

@Conaclos
Copy link
Member

@emab, If we follow the suggestion of @ematipico, I propose noTsIgnoreComment or noIgnoreTsComment.

@ematipico
Copy link
Member

If anyone is interested, this issue has become available

@JoshuaKGoldberg
Copy link

👋 @unvalley are you still planning on contributing this rule?

Context: I'm writing an article for learningtypescript.com on all the TS comment directives. It's going to include best practices for them, including lint rules to prefer @ts-expect-error (#713 (comment)) and enforcing an explanatory comment. I'd like to include a link to all linters that implement this issue's rule, and can wait a bit to publish if you think this will get implemented soon.

@unvalley
Copy link
Member Author

@JoshuaKGoldberg
Hi, thank you for reaching out. I really enjoy reading your blog. I feel a bit bad about keeping you waiting until this issue gets closed, so if possible, it would be great if you could mention in your blog that Biome is tracking this issue. Also, once this lint rule is shipped, I’d love to submit a PR to your blog post!

@JoshuaKGoldberg
Copy link

You got it, and thanks! It's been really encouraging seeing all the progress here. Looking forward to it! ❤️

@ematipico ematipico assigned ematipico and unassigned unvalley Nov 22, 2024
@ematipico
Copy link
Member

I'll work on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

No branches or pull requests

6 participants