Skip to content

Convert all variant analysis commands to typed commands#2191

Merged
robertbrignull merged 9 commits intomainfrom
robertbrignull/variant_analysis_commands
Mar 22, 2023
Merged

Convert all variant analysis commands to typed commands#2191
robertbrignull merged 9 commits intomainfrom
robertbrignull/variant_analysis_commands

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Please check each individual commit because I've done one commit per command.

This did actually find some places where our command arguments were incorrect. For the codeQL.monitorVariantAnalysis and codeQL.autoDownloadVariantAnalysisResult commands we were accepting a cancellation token parameter, but this parameter was not being passed in practice when we were invoking the commands. This meant the type of the argument was incorrect, but the commands already accounted this by checking if the token was undefined and ignoring it if so. Therefore this should be a behaviour-preserving change because the cancellation token was in fact always undefined already.

For codeQL.loadVariantAnalysisRepoResults I chose to give it a return type because this means we can use the existing loadResults method without having to define a new method that just ignores the return value. But I could do this if you think it would be better.

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 force-pushed the robertbrignull/variant_analysis_commands branch from 4a9708d to 1678732 Compare March 20, 2023 17:36
@robertbrignull robertbrignull force-pushed the robertbrignull/variant_analysis_commands branch from 1678732 to 0c9df6e Compare March 20, 2023 17:45
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, I just have 1 very minor suggestion

Comment on lines +5 to +10
import { RepositoriesFilterSortStateWithIds } from "../pure/variant-analysis-filter-sort";
import {
VariantAnalysis,
VariantAnalysisScannedRepository,
VariantAnalysisScannedRepositoryResult,
} from "../variant-analysis/shared/variant-analysis";
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.

Can we make these type imports like all of the above imports just so we can see at once that there are no runtime circular dependencies?

@robertbrignull robertbrignull merged commit e55fb8c into main Mar 22, 2023
@robertbrignull robertbrignull deleted the robertbrignull/variant_analysis_commands branch March 22, 2023 11:56
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