Move language and pack to top level of variant analysis object #3287
Conversation
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Makes sense to pull them up a bit! Thanks.
| */ | ||
| export async function generateVariantAnalysisMarkdown( | ||
| variantAnalysis: Pick<VariantAnalysis, "query">, | ||
| variantAnalysis: Pick<VariantAnalysis, "language" | "query">, |
There was a problem hiding this comment.
Not a problem necessarily introduced in this PR, but I would prefer to not use Pick if we're going to be getting more than 1 item, as it gradually becomes silly (e.g. https://github.com/github/vscode-codeql/pull/3287/files#diff-ba6fe1916e2a1bce3900880797ea49539e39f9f76c431c2742a4ca5926643b26R50)
There was a problem hiding this comment.
For this PR I'll just keep it simple and add the extra field. I don't feel this PR crosses any new silliness-thresholds that weren't already crossed.
I'm not really sure why we're using Pick here (or the place you linked). I'm not sure what benefit it gives us in terms of ease of writing or maintainability, over taking the whole object. Perhaps it makes craft data in the tests easier, but we have createMockX methods for that generally.
|
I dug into the CI failure and uncovered a existing bug in the tests that we just didn't notice until now. I've opened #3290 to separate the test fix from this change. |
|
I found another case of us mixing internal and DTO types in the tests. I've opened #3293 for this one. |
|
At last the tests are green! 🎉 |
A slimmed down version of #3285 that doesn't modify the
queryobject. The idea being to split up that change and make it easier if we do want to do it or something similar to it in the future.My reasoning on
packandlanguageis that they're really properties of the variant analysis rather than properties of the query. This becomes more true when you consider multiple queries, but I think it's still true now.I'm deliberately not modifying the query history format in this PR. We could do, but since it's a lossless transformation in both directions we can just leave it. Or we could save the query history update for when we add in multiple query support and batch those changes together.
Checklist
ready-for-doc-reviewlabel there.