Skip to content

Convert call sites to use typed commands (part 1)#2217

Merged
robertbrignull merged 9 commits intomainfrom
robertbrignull/use_app_commands_1
Mar 23, 2023
Merged

Convert call sites to use typed commands (part 1)#2217
robertbrignull merged 9 commits intomainfrom
robertbrignull/use_app_commands_1

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR converts some places that call commands to use typed commands. This isn't everything but I'll try to open a few smaller PRs instead of one giant PR. That way it'll be easier to review and easier to adapt thoughts and ideas before getting too deep into things.

Each commit converts a single file, so if you want to review commit-by-commit then that is an option.

This PR involves:

  • Documenting new builtin commands whenever we encounter a new one.
    • I'm working out parameter types from the usages we have in the codebase and from the VS Code docs.
  • Passing either App or AppCommandManager to wherever it is needed.
    • When deciding between the two, my thought process so far has been that when we're constructing a long-lived class like VariantAnalaysisManager then give it the full App, but when it's a standalone function that triggers commands but doesn't do anything else then give it just the AppCommandManager.
    • I'm happy to do things differently if there's tangible benefit. It's also worth noting we can change arguments to functions easily, so none of these decisions will block progress in the future.

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 11:00
@robertbrignull
Copy link
Copy Markdown
Contributor Author

Ugh, lots of test compilation errors. Somehow I never spot these locally. 😞

@robertbrignull robertbrignull force-pushed the robertbrignull/use_app_commands_1 branch from 10e9eb7 to 14dc391 Compare March 23, 2023 11:45
@robertbrignull
Copy link
Copy Markdown
Contributor Author

Test should all be fixed now. Things compile for me locally. I've rebased the changes into the original commits, so you can see which changes necessitated which other changes.

// Builtin commands where the implementation is provided by VS Code and not by this extension.
// See https://code.visualstudio.com/api/references/commands
export type BuiltInVsCodeCommands = {
"codeQLDatabases.focus": () => Promise<void>;
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.

Perhaps add a comment for this command? It's not entirely obvious if you're just looking at the file that this is a command added by VS Code on custom views.

@robertbrignull robertbrignull merged commit 6bcfdda into main Mar 23, 2023
@robertbrignull robertbrignull deleted the robertbrignull/use_app_commands_1 branch March 23, 2023 13:18
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.

2 participants