Add prototype implementation for codeQL.runVariantAnalysisPublishedPack#3245
Conversation
robertbrignull
force-pushed
the
robertbrignull/multi-query-published-pack
branch
from
485d5c1 to
174a38a
Compare
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Nice, LGTM!
Do you think it's worth adding an integration test to cover the new functionality? Can be a separate PR/Issue.
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
LGTM to me as well, I just have some non-blocking improvements that I think could be made.
| ); | ||
| } | ||
|
|
||
| return withProgress((progress, token) => |
There was a problem hiding this comment.
I haven't tried running this yet, but would it be helpful to start the progress earlier since quite some work is being done is this method already (downloading the pack, resolving the queries and then resolving query metadata for all queries)?
There was a problem hiding this comment.
Yes, we can do that. We'll have to do some progress report updating schenanigans because the later code sets a different maxStep value, but we can also make that work.
I do wish we had more powerful progress reporting classes which could do this common stuff for us and not require manually putting the right numbers and risking them all getting out of sync. It's a piece of tech debt I hope to get to eventually.
|
I've address the review comments (thank you @koesie10, all of your points were very helpful) and tested it again locally. Everything is working and it hasn't broken existing variant analysis flows as far as I can tell, so I'll go ahead and merge so we can keep moving. |
Yes, I think this would be useful. I'll make an issue so we do this soon. |
3 participants
Implements the
codeQL.runVariantAnalysisPublishedPackcommand. This can likely still be considered a prototype implementation, but it seems to work quite well. Of course right now it fails saying you can't run multiple queries, but that will be fixed in the future.Checklist
ready-for-doc-reviewlabel there.