-
Notifications
You must be signed in to change notification settings - Fork 399
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
Resolve/ignore eslint warnings #1095
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
- Coverage 69.50% 69.31% -0.20%
==========================================
Files 13 13
Lines 1279 1284 +5
Branches 376 380 +4
==========================================
+ Hits 889 890 +1
- Misses 319 320 +1
- Partials 71 74 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
One slight tweak as an alternative: what if we disabled certain rules for test files? no-explicit-any
, no-non-null-assertion
and func-names
seem like heavy-handed rules for tests. We can add test-file-specific rules easily now to a specific section of the eslintrc.js
file.
This must have been a lot of work. Thanks for taking it on! I wonder if @filmaj's comment will help keep this maintainable in the future by setting test-specific rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan going forward if we opt to suppress the violation of rules we've decided on instead of fixing them? Just no new warnings going forward? My main concern with side-stepping/suppressing them is that they'll end up like TODOs generally do: out of sight and out of mind.
@filmaj @misscoded For future reference, we had 240 warnings but actually all of them on the main code side are "no-explicit-any" and "no-non-null-assertion". Regarding "no-explicit-any", changing As for "no-non-null-assertion", we already have many TODO comments about it. For instance, |
Summary
This pull request resolves/ignores 240 eslint warnings. For tests, this PR basically ignores warnings for entire files. As for the library code, it depends on what the warnings are.
Requirements (place an
x
in each[ ]
)