-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
feat(biome_js_analyze): noTsIgnoreComment add initial rule #2576
base: main
Are you sure you want to change the base?
Conversation
Bit of a work in progress. Not sure if I should include an action to remove the comment, or replace with |
CodSpeed Performance ReportMerging #2576 will not alter performanceComparing Summary
|
I'm guessing the performance hit is due to using I wasn't sure how to directly find only comment stings. Should I be using the semantic model instead? |
It's a false positive, don't worry |
No. You only need to query the CST, comments are trivia and they have no part in the semantic model. However, you have to widen the query. Comments can be anywhere! Technically, you'd need to query any node |
Okay cool, I thought that was the case!
Ah so right now I'll be missing comments within module items I suppose? I was hoping I'd be able to match on a comment line, but since it's trivia I assume I'll need to search for any node and collect matching ts ignore trivia's? |
Yeah, that's right. There was this old initial implementation that you could use as a source of inspiration: https://github.com/rome/tools/pull/2811/files |
Thanks for sharing that. I guess my question now is - are the performance issues mentioned now no longer a problem? Also:
Am I correct in thinking we can now query syntax tokens? Should I be querying these in order to catch the edge cases? Quite new to this world so apologies if these are simple questions! |
Hey @ematipico, we released a feature to ignore benchmarks that have a tendency to be flaky. This is not ideal, but sometimes the code used in some benchmarks is by definition not deterministic, so removing them from the report is preferable to avoid noise. Ignored benchmarks are still measured at every run though, and their history can still be accessed on CodSpeed 😉 Let me know if that works for you! |
Hey @adriencaccia! That's an amazing feature, thank you for implementing that, I will start ignoring flaky benchmarks now :) |
You can give it a try, let's see how it goes! |
@emab are you still interested in this PR? |
Summary
Implements
noTsIgnoreComment
rule, which prevents the use of@ts-ignore
.nursery/noTsComment
-typescript-eslint/ban-ts-comment
#713Test Plan
TBC