Create wrapper runCliJson#3981
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR introduces a runCliJson helper in src/codeql.ts to standardize “run CLI command + parse JSON output” behavior, reducing duplicated JSON.parse logic across CodeQL CLI wrapper methods.
Changes:
- Add a generic
runCliJson<T>wrapper aroundrunClithat parses JSON output. - Refactor several CodeQL wrapper methods (e.g.,
resolve languages,resolve build-environment,resolve queries,resolve database,version) to userunCliJsoninstead of inlined parsing.
Show a summary per file
| File | Description |
|---|---|
| src/codeql.ts | Adds runCliJson and refactors multiple CLI wrapper methods to centralize JSON parsing. |
| lib/entry-points.js | Generated JS output update (content excluded from diff; also not generally reviewed for lib/). |
Review details
Files excluded by content exclusion policy (1)
- lib/entry-points.js
- Files reviewed: 1/2 changed files
- Comments generated: 2
- Review effort level: Low
3f66d8e to
6cdc606
Compare
| } | ||
| result = await runCliJson<VersionInfo>( | ||
| cmd, | ||
| ["version", "--format=json"], |
There was a problem hiding this comment.
Could the --format=json or --format=betterjson argument be added automatically by runCliJson?
There was a problem hiding this comment.
That would be nice! Unfortunately, there doesn't seem to be consistency between all of the calls of runCliJson:
getVersion,resolveDatabaseuse--format=jsonresolveLanguagesuses--format=betterjsonresolveBuildEnvironmentuses nothing??resolveQueriesStartingPacksuses--format=startingpacks
| // TODO: Unconditionally include `--filter-to-languages-with-queries` | ||
| // once CODEQL_MINIMUM_VERSION is at least v2.23.0 | ||
| // — the first version to support this flag. |
There was a problem hiding this comment.
Elaborating on my comment on the other PR, what does this comment have to do with the changes here? Just a drive-by improvement?
There was a problem hiding this comment.
what does this comment have to do with the changes here? Just a drive-by improvement?
Nothing, really—a "drive-by improvement" is a nice way to word it. But, it seemed more relevant here than in #3950. I'm fine to delete it, but I think it adds some value.
fe0c69f to
8ff2a75
Compare
mbg
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback! I am slightly in two minds about 8ff2a75. On one hand, the type of runCliJson is more accurate now, but on the other hand, we still just assert the types (and that now makes the rest of the code uglier). If the intention is not to perform any validation on the JSON as part of this PR (which is OK, since that's the status quo) then the previous version where runCliJson returned Promise<T> was probably better.
8ff2a75 to
ec025cd
Compare
Yes, attempting to validate the output—while justified and correct—is beyond the scope of this simple PR. I'll delete the commit. |
ec025cd to
d5b8046
Compare
Summary
A few of the CLI commands emit JSON. This PR creates a
runCliwrapper function calledrunCliJsonthat expects JSON output from the CLI command. This change centralizes and reduces the number of JSON-parsing call sites incodeql.ts, making the code more readable and D.R.Y.er.This PR is a prerequisite for #3950.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.upload-sarifaction.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist