Skip to content

Add support for quick eval count to the query runner#2417

Merged
alexet merged 2 commits intomainfrom
alexet/add-quick-eval-count2
May 30, 2023
Merged

Add support for quick eval count to the query runner#2417
alexet merged 2 commits intomainfrom
alexet/add-quick-eval-count2

Conversation

@alexet
Copy link
Copy Markdown

@alexet alexet commented May 11, 2023

This only adds support internally to the query runner, without any UI support. I will add basic support (command palette only) in a separate PR.

I'm not completely happy about the api as we switch between an enum and booleans. The issue is that before running for quick eval we simply have a boolean and afterwards we have an optional position. It should then really become a optional (position and count enabled) but in a bunch of places we need backwards compatibility as we persist it as json. So we end up with an invalid state (no quick eval position, but quickEvalCountOnly is set), and a difference between the initial enum and the later boolean state. However I couldn't think of much better as I don't want to add yet more boolean arguments to the initial query.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
    • No user visible changes in this PR
  • Issues have been created for any UI or other user-facing changes made by this pull request.
    • No user visible changes in this PR
  • [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.
    • No user visible changes in this PR

This only adds support internally to the query runner,
without any UI support.
@alexet alexet requested a review from dbartol May 11, 2023 14:54
@alexet alexet requested review from a team as code owners May 11, 2023 14:54
@alexet alexet marked this pull request as draft May 11, 2023 15:21
@alexet alexet force-pushed the alexet/add-quick-eval-count2 branch from 396e1aa to 0c83319 Compare May 12, 2023 15:07
@alexet alexet force-pushed the alexet/add-quick-eval-count2 branch from 0c83319 to 537bb29 Compare May 12, 2023 15:08
@alexet alexet marked this pull request as ready for review May 12, 2023 17:19
@alexet alexet requested review from aeisenberg and removed request for dbartol May 26, 2023 13:47
@alexet alexet assigned aeisenberg and unassigned dbartol May 26, 2023
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.

I don't fully understand the context of this change, but it looks generally safe and fine.

Comment on lines +74 to +76
* This is only supported by the new query server
* but it isn't worth having a separate type and
* it is fine to have an ignored optional field.
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.

👍🏼

@alexet alexet merged commit 22172d5 into main May 30, 2023
@alexet alexet deleted the alexet/add-quick-eval-count2 branch May 30, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants