Skip to content

Convert call sites to use typed commands (part 3)#2229

Merged
robertbrignull merged 8 commits intomainfrom
robertbrignull/use_app_commands_3
Mar 24, 2023
Merged

Convert call sites to use typed commands (part 3)#2229
robertbrignull merged 8 commits intomainfrom
robertbrignull/use_app_commands_3

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Follows on from earlier PRs and converts some more places to call typed commands.

The only non-trivial bit of this PR is around VariantAnalysisMonitor:

  • This class already took credentials, but in this case I decided to instead provide a full app and moved it to the constructor instead of the monitorVariantAnalysis method.
  • The tests were asserting that the codeQL.autoDownloadVariantAnalysisResult command was called, but then also asserting that the VariantAnalysisManager. autoDownloadVariantAnalysisResult method was called. This only works if it's actually executing commands rather than using a mock, and is also not very necessary. I decided to remove these and just check that the command was called, and this made the tests a lot simpler.

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.

@robertbrignull robertbrignull requested review from a team as code owners March 23, 2023 16:44
this.push(
config.onDidChangeConfiguration(() =>
commands.executeCommand("codeQL.restartQueryServer"),
app.commands.execute("codeQL.restartQueryServer"),

Check warning

Code scanning / CodeQL

A VS Code command should not be used in multiple locations

The codeQL.restartQueryServer command is used from 3 locations
Comment on lines +31 to +32
private readonly ctx: ExtensionContext,
private readonly app: App,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels strange to be passing in both the context and the app. I believe both of the properties that this class accesses on the context (extensionMode and extensionUri) also exist on the app as mode and extensionPath. I think it would be best to switch to those in this PR as well, even though the change is unrelated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. I didn't really have it clear in my head what the point of ExtensionContext was and how it relates to the app, but I see now the app can replace the extension context in some places. The context is stored as a field on the app, so it can act as a platform-independent interface to those values.

I can take a look (not in this PR) to see if there are more places where we pass both types and if it could be simplified.

@robertbrignull robertbrignull merged commit d4a8eb9 into main Mar 24, 2023
@robertbrignull robertbrignull deleted the robertbrignull/use_app_commands_3 branch March 24, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants