chore: enable no-non-null-assertion internally, excluding tests #8019
Conversation
|
Thanks for the PR, @JoshuaKGoldberg! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| line, | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Dead code 🔪 #8021
There was a problem hiding this comment.
I do wonder if this rule is good for our codebase - this PR adds 114 disable comments across our packages - which is a large amount! (Plus the number in the files that have the rule completely disabled via unlimited comment).
Perhaps there's something we can do better here in regards to the types to make things less "hacky"?
Perhaps some of these should be converted to runtime checks (eg we have the nullThrows util)
|
Yeah especially around the token stuff, agreed. Will draft & put in my queue... |
|
FWIW, this is one of the rules I passionately hate, and I think it just adds a lot of noise to most codebases that are not written by novices. I'm okay to have it just for dogfooding, though. |
|
I'll wait to merge these changes until the flat config work lands. (they haven't been approved, but just in case they are) Edit: ✔️ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8019 +/- ##
==========================================
- Coverage 87.90% 87.75% -0.16%
==========================================
Files 397 398 +1
Lines 13844 13828 -16
Branches 4074 4068 -6
==========================================
- Hits 12170 12135 -35
- Misses 1372 1396 +24
+ Partials 302 297 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| return nullThrows( | ||
| context.sourceCode.getTokenAfter(node.key), | ||
| NullThrowsReasons.MissingToken(']', 'key'), | ||
| ); |
There was a problem hiding this comment.
this pattern is so common - i wonder if we should create a utility to wrap it up and make it even shorter?
eg getRequiredTokenAfter(sourceCode, node.key, 'optional message')
There was a problem hiding this comment.
Hmm, tried this out - but in order to get the same precision of logs we'd need to still pass in ']' and key as well.
return getRequiredTokenAfter(
context.sourceCode,
node.key,
']',
'key',
);I feel like I'd rather get this merged in now, then look at more options in a followup. WDYT?
There was a problem hiding this comment.
I wonder if we should just say "fuck it" and create an API without the message.
It makes the end user experience worse cos more vague error messages - but it would be much cleaner and better devx.
There was a problem hiding this comment.
I don't really mind having the extra verbosity for our devx, personally. It's been forcing me to think more carefully about what we're actually asserting on.
PR Checklist
Overview
Moves the disable of @typescript-eslint/no-non-null-assertion to test files. Adds a whole bunch of
disablesnullThrowsto the remaining violations. I think we have so many because we're normally pretty good at not adding unnecessary!s, so any!that was added was generally there for a good reason.Also adds a unit test to the actually-testable case in
nullThrows.