Wip/ts null checks

[corasaurus-hex]

What has Changed?

Handled strict null checks for when we get the active text editor and for when we get the mirror doc.

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Added to or updated docs in this branch, if appropriate
• Tests
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
  • Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test.
• [~] Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • [~] If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • [~] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• [~] Created the issue I am fixing/addressing, if it was not present.
• Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
• Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

[PEZ]

Thanks for taking care of Calva quality!

I need some help to understand the implications of the changes. I see that mustGetActiveEditor throws. So what will happen at some call site when there is no active editor?

Is the plan to get rid of util.getActiveEditor and mirror-doc.getDocument? If so, maybe name them legacyGet..., or dangerouslyGet... or something and use the shorter name for the new and proper functions?

[PEZ]

I need some help to understand the implications of the changes. I see that mustGetActiveEditor throws. So what will happen at some call site when there is no active editor?

I now saw that you had mentioned this on slack and that it will just retain the current behaviour. That's cool. Baby steps.

[corasaurus-hex]

I need some help to understand the implications of the changes. I see that mustGetActiveEditor throws. So what will happen at some call site when there is no active editor?

activeTextEditor can be undefined and accessing a property on undefined throws an error. So if the calling code isn't checking if activeTextEditor is undefined then I'm calling mustGetActiveTextEditor instead. This way we get an error explaining that the issue is that we don't have an activeTextEditor instead of getting TypeError: Cannot read properties of undefined. The other upside to doing it this way is that it results in the function only returning a TextEditor and not TextEditor | undefined which makes subsequent uses of the activeTextEditor a lot less painful.

Is the plan to get rid of util.getActiveEditor and mirror-doc.getDocument? If so, maybe name them legacyGet..., or dangerouslyGet... or something and use the shorter name for the new and proper functions?

I don't see a reason to get rid of them, they're still useful in a few places where the code is checking if it's undefined. If we eventually remove all calls to the functions that can return undefined then sure.

I now saw that you had mentioned this on slack and that it will just retain the current behaviour. That's cool. Baby steps.

It's mostly the same behavior. Let me give a few examples.

1. In this section of the code it calls to get the mirrored document and then immediately calls getCursorToken on it, which would already cause error if the mirrored document was missing. Throwing an error in mustGetDocument is basically the same behavior as before except it has a better error message now.
2. In this section of the code I moved the fetching of the editor close to where it's first used so that if an error is thrown it's thrown near the spot it used to be thrown.
3. And in sections of code like this things get a little more difficult to suss out what the correct behavior should be. I made the assumption that we wouldn't want it to evaluate code if there is no activeTextEditor (and that it wouldn't happen in practice, but we'll see about that). Similar choices were made here, here, here, and maybe other places.

So it's mostly the same behavior as before but it's not impossible that it changes the behavior in some important way that I'm not aware of.

[corasaurus-hex]

Also, maintenance-wise, is it important to create an issue for these sorts of code quality improvements?

[PEZ]

Thanks! And thanks for taking your time to explain the implications to me. It all sounds perfect and wonderful to me. As I've mentioned before we have quite a lot of strange errors where there is very hard for users and us to try figure out what has actually gone wrong.

Also, maintenance-wise, is it important to create an issue for these sorts of code quality improvements?

The important part is the changelog entry. When a users experiences a regression it is very handy with good descriptions on what has changed when. And that changelog should ideally be linked to somewhere where you can read more about it. The best place is usually the problem statement, so the issue. In this case you can link to this PR instead, if you like.

[bpringe]

In this section of the code it calls to get the mirrored document and then immediately calls getCursorToken on it, which would already cause error if the mirrored document was missing. Throwing an error in mustGetDocument is basically the same behavior as before except it has a better error message now.

This is great. I think I've seen errors about calling getTokenCursor of undefined in logs before, so this should give us more info to fix issues like this.

[corasaurus-hex]

I'm pretty sure this is ready to merge at this point?

[PEZ]

Thanks! LGTM.

I left a nitpick about the changelog.

I still think muestGet is not going to be as discoverable in the module API as something that begins with get. In Clojure I would solve it with some symbol slapped to the end of the symbol name. but here I am out of better ideas.

[corasaurus-hex]

It could be getDocumentOrError? That would be a little more discoverable, I think? Or perhaps we make strict the default and make the function that can return undefined be something like maybeGetDocument or tryToGetDocument or something similar?

[PEZ]

I like both suggestions. The latter is my favorite, I think.