Skip to content

Remove ProgressCallback / CancellationToken arguments where they aren't used#2525

Merged
robertbrignull merged 4 commits intomainfrom
robertbrignull/sendRequest_progress
Jun 20, 2023
Merged

Remove ProgressCallback / CancellationToken arguments where they aren't used#2525
robertbrignull merged 4 commits intomainfrom
robertbrignull/sendRequest_progress

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

The idea here is to remove ProgressCallback or CancellationToken arguments where they ultimately aren't used. This involves tracing the execution into the query server. I did this by looking in the CodeQL CLI repository in the EvaluationServer class, to see if either of those arguments are used in the implementation.

The affected query server commands are:

  • clearCacheInDatabase
  • registerDatabase
  • deregisterDatabase

My understanding is that the state of whether or not a query server command utilises progress reporting is unlikely to change in the foreseeable future. So these assumptions about whether commands use these arguments or not shouldn't go out of date too quickly.

Where they are not used, I've removed them from the QueryRunner interface, and then followed the chain of removing unused variables as far as possible.

I've tried testing this by running these three operations, and everything seems to be working just as before. In some cases we intentionally use withProgress but then don't make any progress updates to it. In the UI this works and it looks like a progress bar that animates to show activity but doesn't show a particular percentage.

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 June 19, 2023 16:24
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Thanks. This makes sense to me and I think is a good cleanup.

tokenSource.token,
database,
);
await this.showProgress({
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.

Upon re-reviewing my own PR, I think we should keep this one. I'm not 100% sure if it worked before, but the intention was to do a manual progress update to show that we're removing the database.

@robertbrignull robertbrignull enabled auto-merge June 20, 2023 11:34
@robertbrignull robertbrignull merged commit 23173bf into main Jun 20, 2023
@robertbrignull robertbrignull deleted the robertbrignull/sendRequest_progress branch June 20, 2023 13:16
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