Further work on TypeScript strictNullChecks #1581
Conversation
corasaurus-hex
force-pushed
the
cora/further-ts-null-checks
branch
from
abe8606 to
822fe17
Compare
corasaurus-hex
force-pushed
the
cora/further-ts-null-checks
branch
from
a5ec5a8 to
ff8a168
Compare
corasaurus-hex
force-pushed
the
cora/further-ts-null-checks
branch
from
ff8a168 to
e9371a3
Compare
With strict nulls on these arrays mostly become never[] and so we must explicitly set their type.
These type assertions are for when we have strictNullChecks enabled but eslint sees them an not needed when we have it disabled. In order for it to work both ways, enabled and disabled, we need to also disable this additional eslint rule for this line.
|
I tried to keep the behavior the same. There are 242 remaining errors (down from 750-ish!). There is probably some more low-hanging fruit remaining if I sift through some more but very soon we'll need to start revamping pieces to support strictNullChecks. |
corasaurus-hex
changed the title
|
I've started reviewing, but need to stop for now. I'll try to finish tomorrow. |
|
Thanks for doing this, @corasaurus-hex ! I get dizzy reading through all the changes and can just imagine what an effort it must be providing. I get dizzy reading through all the changes. It all looks very sane to me. I only see a few places where I start to worry about the current behaviour being kept. Not because I see any error, but because I can't reason about the changes clear enough to understand the implications from reading. This says much more about the current code than the changes, so there is nothing we can do about it in this PR. I will run with this VSIX today for work. If that works as usual, then I will vote for merge. And we'll get @bpringe's review results in tomorrow, so hopefully we'll merge tomorrow! 🍾 (I like to celebrate in advance. The worst case scenario there is an extra celebration, right?). |
|
I merged latest dev into this, so that my testing gets more real-world. 😄 I do hope I didn't merge away any of your changes, but it merged pretty cleanly at least. |
| @@ -86,7 +93,7 @@ function getCurrentArgsRanges(document: TextDocument, idx: number): Range[] { | |||
| (previousRangeIndex > 1 && | |||
| ['->', 'some->'].includes(previousFunction)) || | |||
| (previousRangeIndex > 1 && | |||
| previousRangeIndex % 2 && | |||
| previousRangeIndex % 2 !== 0 && | |||
There was a problem hiding this comment.
Was this check for the modulus result just missing before?
There was a problem hiding this comment.
yep! it worked before because 0 is false-y. however, we're assigning the result of this condition to a boolean variable with the previous code if the modulus was 0 then the result of this condition would be 0 instead of a boolean
| export function isResultsDoc(doc: vscode.TextDocument): boolean { | ||
| return doc && path.basename(doc.fileName) === RESULTS_DOC_NAME; | ||
| export function isResultsDoc(doc?: vscode.TextDocument): boolean { | ||
| return !!doc && path.basename(doc.fileName) === RESULTS_DOC_NAME; |
There was a problem hiding this comment.
What's the difference in using double negation here vs just doc && ...?
There was a problem hiding this comment.
with doc && ... if doc was undefined then this function would return undefined. the function type says it returns a boolean, though, and while we could make it boolean | undefined I prefer to keep the types more constrained and simpler if I can
|
@PEZ I checked out the merge, all seems well. thanks for doing that! |
|
TIL what the non-null assertion operator is 😄 . Anyway, let's merge! |
Whoops. I hope that merge was okay. 😅 |
What has Changed?
My Calva PR Checklist
I have:
devbranch. (Or have specific reasons to target some other branch.)published. (Sorry for the nagging.)[Unreleased]entry inCHANGELOG.md, linking the issue(s) that the PR is addressing.ci/circleci: buildtest.npm run prettier-format)npm run eslintbefore creating your PR, or runnpm run eslint-watchto eslint as you go).Ping @PEZ, @bpringe