Move bundle types and helpers out of setup-codeql.ts - #4167
henrymercer merged 2 commits into
Conversation
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Copilot review overview
🟢 Approval recommended
The refactor preserves existing behavior and public call patterns with relevant test coverage.
Review effort: Balanced
Findings: None
Refactors bundle-related types and helpers into focused modules without changing behavior.
Changes:
- Moves bundle naming and download-source definitions into codeql-bundle.ts.
- Moves per-language fallback logging into per-language-bundles.ts.
- Preserves existing setup-codeql.ts exports for compatibility.
| File | Description |
|---|---|
| src/setup-codeql.ts | Delegates to relocated helpers while preserving exports. |
| src/codeql-bundle.ts | Defines bundle naming and download-source types. |
| src/per-language-bundles.ts | Adds the fallback-warning helper. |
| lib/entry-points.js | Generated output; excluded from review by policy. |
- lib/entry-points.js
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
LGTM. One minor comment, but I imagine that you'll have follow-up PRs to this one, so feel free to address it in one of those and merge this one as-is.
Sorry, something went wrong.
| /** Explains why an eligible per-language bundle is being replaced by a combined bundle. */ | ||
| export function logPerLanguageBundleFallback( |
There was a problem hiding this comment.
Minor: Since this is just one possible reason why we might use a combined bundle rather than a per-language one, the "eligible" in the comment is doing a lot of heavy lifting and the function name doesn't make it clear at all. Consider
| /** Explains why an eligible per-language bundle is being replaced by a combined bundle. */ | |
| export function logPerLanguageBundleFallback( | |
| /** Logs that we are using a combined bundle because no per-language bundle was found. */ | |
| export function logNoPerLanguageBundle( |
Sorry, something went wrong.
There was a problem hiding this comment.
We'll only ever log this when we expected there to be a per-language bundle, hence the "fallback" in the function name, but the log message doesn't mention that one was expected. I'll update this.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the context regarding the circumstances of when logPerLanguageBundleFallback would be called at runtime, but that context is not necessarily obvious when looking at the source code here and from that angle I think my comment applies regardless?
Sorry, something went wrong.
Move CodeQLDownloadSource and bundle asset naming into codeql-bundle.ts, and the per-language fallback warning into per-language-bundles.ts, so that other modules can use them without importing setup-codeql.ts. The moved naming function takes the platform as a parameter, so setup-codeql.ts now passes the same platform it uses to pick a per-language bundle. setup-codeql.ts still re-exports CodeQLDownloadSource. No behavior changes.
Risk assessment
Low risk: Refactor with no behavior changes, covered by existing unit tests.
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
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