Avoid having functions that aren't at the top level in extension.ts

[robertbrignull]

I find it confusing to read the code in extension.ts and a part of that is because lots of the functions are not defined at the top level but are defined within another function and thus implicitly reference a bunch of local variables. This does make the code smaller but it makes it much harder to reason about what references what and what order things happen.

This PR attempts to pull all suitable functions outs to the top level and give them extra arguments whenever they need to access data that they previously had access to implicitly. This draft PR currently isn't what I'd call perfect code and it could definitely be made cleaner and nicer, but I want to first get a general overview of if people think this would be a beneficial change or not.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[aeisenberg]

I haven't reviewed this yet, but just wanted to comment first. This area does not have much testing and is quite complex. Even testing a lot of this manually will be very difficult since it depends on rate limiting and some global state.

The difficult part is mostly around checking for updates and downloading new CLI versions. A great end goal would be to refactor this enough so that we can create unit tests (mocking out the actual download and installation behaviour) and ensure that the logic does what we want.

But, this PR has much simpler goals.

[aeisenberg]

This PR looks pretty safe to me. Just before merging can you check a few manual paths:

• Update from an old CLI version still works.
• Rate limiting on update checks still works (ie- on restart, the check for updates is avoided if it already happened recently)
• If there is a CLI on your PATH, then updates are not checked for.

There are a few other things to try, but I think that if these still work, we're most likely safe for the others. Thanks for taking this on.

[robertbrignull]

Thanks @aeisenberg for reviewing and the suggestions around testing limitations and manual testing. I will do some manual tests. I also plan to go through the code changes and make the git history / diff a bit prettier and easier to understand, so it's clear which code is just moving and which bits are changing as part of it.

[robertbrignull]

I've re-done this PR but done each move as a separate commit. I've had to include changes to function interfaces in with the moves to make the code compile, but I think the diff is still understandable. I suggest looking at each commit individually and using git show --color-moved --color-moved-ws=allow-indentation-change to see how code has moved or been changed. This coloring makes it a lot clearer.

[koesie10]

Thanks for splitting this up, this makes it really easy to review, especially with the command you suggest! All changes look sensible to me, I just have a minor comment about some argument ordering.

[koesie10]

I think this would be better if the selectedQuery was the last argument since it's the argument to the command. We now have (from extension, from user, from extension), while I would expect everything from the extension to be grouped together.

[robertbrignull]

Good point. I've swapped these two around.

[elenatanasoiu]

This is great. One step closer to breaking down this massive file.

[robertbrignull]

I've done some manual testing of this (with the extension built on actions from this branch) and it seems to be ok. It's starting up ok and everything seems to be happening as expected, which is what you'd hope since the code has just moved around. I think the type checking should keep us pretty safe. The only thing I can think of that would make it fail is if an argument is non const and thus it's reference changes over time, but I can't see any of these. All the new arguments we're passing are const values.

I did encounter an error when I was upgrading to test where the managed CLI was somehow deleted. I don't know what happened here and I couldn't reproduce it by down+upgrading again. It was fixed by manually running the CodeQL: Check for CLI updates command, but it wasn't automatically installing the CLI because it was still on cooldown / rate limited. I think this behaviour is not changed by this PR so I'll merge this and open an issue to investigate this behaviour more.

[robertbrignull]

To add to the above, my best guess is that _installedReleaseStateKey somehow got cleared out of globalState when I was installing the new VSIX. I don't know how this happened. It could be VSCode was shut down while it was doing something, because we do clear that value and reinstate it during some operations.