Fork me on GitHub
Tomas Brejla16:02:22

Hi, what's the story behind changing dedent line action to shift+tab? It kinda seemed to make sense being mapped to ctrl+shift+i, as is paired nicely with indent line on ctrl+i.

Tomas Brejla16:02:19

Now that it's on shift+tab , it's tempting to try to pair it with tab, which obviously does somthing bit different than indent line (ctrl+i).


I should maybe have left it at ctrl+shift+i. It was more that shift+tab became available when I removed the default keybinding for Infer parens. I was frequently inferring parens instead of dedenting the line. 😃

Cora (she/her)22:02:50

I'm working on enabling strict null checks and I need to identify what we want to happen when different undefined/null values are hit, and this mostly speaks to what the invariants are. more in thread

Cora (she/her)22:02:29

for example, in src/doc-mirror/index.ts, the getDocument function can return undefined. This is called all over the place. If we expect it to always be there then we could throw an exception in the getDocument function when it's undefined and then no call-sites need to handle the potential for an undefined value.

Cora (she/her)22:02:21

another example is vscode.window.activeTextEditor, which may not return an editor. do we want to detect this situation and throw an exception? silently fail?

Cora (she/her)22:02:14

if it doesn't happen in practice that activeTextEditor is missing then we should add a function to get the activeTextEditor that throws an error if it's undefined and thereby simplifies each callsite

Cora (she/her)23:02:54

what about when we're executing commands? vscode.commands.executeCommand returns Thenable<boolean | undefined> but the current code assumes it'll return a Thenable<boolean> -- do we want to check that it's not undefined when executing the command and then raise an error so that other callsites don't need to handle undefined?

Cora (she/her)23:02:56

there are a lot of these sorts of situations in the code and it's hard to figure out where we want to be handling these issues

Cora (she/her)23:02:01

I could just make the same assumptions the current code makes and just throw an error in these situations since they can't handle undefined presently anyways

Cora (she/her)23:02:25

that would certainly require the fewest changes, and then we have places to start improving it more gradually

Lukas Domagala23:02:16

I think throwing on null/undefined in cases where it would be handled wrong is a good start. There are probably too many cases, some of them bugs, to fix all of it at once. Throwing wouldn’t make the situation worse, just make it more explicit. Once all of them have well defined behavior we should probably make a list and try to go through them?


Throwing seems like a good way to avoid issues, but I wonder if we do that with some functions for which we currently handle undefined or null returns in several places, but don’t handle exceptions (or at least not well), then we risk breaking functionality in several places. Are you thinking we’d had try/catches in those locations? Overall I think it’s a good thing to do though, but it might just need to be done in small increments and carefully, testing the functionality touched as we go (writing tests would be great, too). I also might be somewhat missing some implications at the moment.

Cora (she/her)23:02:45

that function is called from 26 locations in the codebase

Cora (she/her)23:02:45

sorry, referenced 26 different times

Cora (she/her)23:02:50

most locations assume that it returns a value every time

Cora (she/her)23:02:59

but in a few places it's handling when the doc isn't there


I think the approach that requires fewest changes to start would be good, then we can adjust things on a case-by-case basis, or maybe a few cases per PR.


I don't think I understand the implications well enough to weigh in with something very useful. 😃 The goal to have explicit management over when undefined is a result sounds great.


About getDocument. VS Code is a bit weird with that surprising things are TextDocuments, so we will often not have a mirror. Will something change at the call sites of getDocument, if we enable strict null checks? How, if so?