Skip to content

Remove unnecessary commands from activationEvents#2153

Merged
norascheuch merged 2 commits intomainfrom
nora/remove-unnecessary-activation-events
Mar 10, 2023
Merged

Remove unnecessary commands from activationEvents#2153
norascheuch merged 2 commits intomainfrom
nora/remove-unnecessary-activation-events

Conversation

@norascheuch
Copy link
Copy Markdown
Contributor

VSCode Docs: https://code.visualstudio.com/api/references/activation-events

From the docs: Your extension becomes activated when the Activation Event happens.. This means only commands that can be triggered from the command palette need to be listed in the activationEvents. All other events can only be executed when the extension is already activated.

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.

@norascheuch norascheuch requested a review from a team as a code owner March 9, 2023 11:16
@robertbrignull
Copy link
Copy Markdown
Contributor

Screenshot 2023-03-09 at 15 27 40

According to the linting in my VS Code we can remove quite a few more. I see the ones currently included in this PR are just the ones related to variant analysis.

I think we can remove all of the indicated events? What do you think? Does your VS Code also give these warnings.

@robertbrignull
Copy link
Copy Markdown
Contributor

robertbrignull commented Mar 9, 2023

We might also be able to remove codeQLDatabases.chooseDatabase. As far as I can tell that command doesn't exist. There are a few codeQLDatabases.chooseDatabaseX for where X is Folder, Achive, Internet, Github. Those commands are also listed and marked as unnecessary, so it's not like there's anything special going on because it's a prefix. I think this is leftover from the command getting renamed or split up and now we can remove it.

Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM!


Re:

According to the linting in my VS Code we can remove quite a few more. I see the ones currently included in this PR are just the ones related to variant analysis.

I think we can remove all of the indicated events? What do you think? Does your VS Code also give these warnings.

This is new from VS Code version 1.74.0, so let's not remove these yet (until we set that as our earliest supported VS Code version). We have an internal issue tracking this 💪🏽

Comment thread extensions/ql-vscode/package.json Outdated
Comment on lines 54 to 55
"onCommand:codeQL.chooseDatabaseGithub",
"onCommand:codeQLDatabases.chooseDatabase",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree with @robertbrignull that onCommand:codeQLDatabases.chooseDatabase can be removed, since it doesn't exist 😄

Suggested change
"onCommand:codeQL.chooseDatabaseGithub",
"onCommand:codeQLDatabases.chooseDatabase",
"onCommand:codeQL.chooseDatabaseGithub",

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.

Yes, good catch @robertbrignull! :)

@robertbrignull
Copy link
Copy Markdown
Contributor

I bow to @shati-patel here 🙇 so please go with her review.

@norascheuch norascheuch force-pushed the nora/remove-unnecessary-activation-events branch from 2494de5 to 275c16d Compare March 10, 2023 08:54
@norascheuch norascheuch merged commit 1fa9767 into main Mar 10, 2023
@norascheuch norascheuch deleted the nora/remove-unnecessary-activation-events branch March 10, 2023 09:23
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