Migrate changetool to pr-checks - #4128
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
19e9e17 to
d235931
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The migration preserves existing behavior, consolidates infrastructure cleanly, and passes the relevant tests and checks.
Review tier: Balanced
Findings: None
What changed in this PR
Migrates changetool into the shared pr-checks workspace while retaining ESM compatibility through .mts files.
Changes:
- Relocates changelog validation code and tests.
- Consolidates dependencies and TypeScript/ESLint configuration.
- Runs migrated tests through the existing PR-checks job.
| File | Description |
|---|---|
.github/workflows/pr-checks.yml |
Removes the redundant changetool test job. |
eslint.config.mjs |
Applies PR-checks linting to .mts files. |
package-lock.json |
Updates workspace and dependency resolution. |
package.json |
Removes the separate changetool workspace. |
pr-checks/changelog/validate.mts |
Relocates changelog validation logic. |
pr-checks/changelog/validate.test.mts |
Relocates and adapts validation tests. |
pr-checks/package.json |
Adds changetool dependencies. |
pr-checks/tsconfig.json |
Includes nested TypeScript and .mts sources. |
pr-checks/validate-changelog.mts |
Relocates and renames the CLI entry point. |
scripts/changetool/package.json |
Removes the obsolete standalone package. |
scripts/changetool/tsconfig.json |
Removes the obsolete standalone configuration. |
tsconfig.json |
Removes the deleted project from exclusions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const filePath = path.join(tmpDir, baseFileName); | ||
| fs.writeFileSync(filePath, contents); | ||
| return await body(filePath); | ||
| return await Promise.resolve(body(filePath)); |
There was a problem hiding this comment.
Isn't this addition redundant and unnecessary? await will already resolve any Promise returned from body, so it's effectively the same as:
return await body(filePath);But, also—now that I look closer—body (currently) never returns anything, so we can simplify it further:
| return await Promise.resolve(body(filePath)); | |
| body(filePath); |
Which means withTmpFile nor its anonymous body functions no longer need be async.
| baseFileName: string, | ||
| contents: string, | ||
| body: (filePath: string) => Promise<T>, | ||
| body: (filePath: string) => Promise<T> | T, |
There was a problem hiding this comment.
Since none of the body instances ever return anything, this can be simplified to:
| body: (filePath: string) => Promise<T> | T, | |
| body: (filePath: string) => void, |
| "2026-01-01-fix-bug.md", | ||
| "---\ncategory: fix\n---\n- Fixed a bug\n", | ||
| async (filePath) => { | ||
| (filePath) => { |
There was a problem hiding this comment.
This change and the other similar ones below are not necessary with the noted simplifications to withTmpFile.
There was a problem hiding this comment.
FYI, this script/tool will also be used for more than just validation, so I wouldn't narrow the name to that singular purpose, but it's fine for now. It can be changed later, if need be.
|
BTW, Thanks for putting this together for me! |
Follow-up to discussion on #4116 which migrates
scripts/changetoolfrom its own project to the commonpr-checksproject to avoid duplicated infrastructure.Although
changetool's two unique dependencies are ESM-only, we simply use the.mtsextension for the migrated source files to avoid switching all ofpr-checksover to ESM, which would require other modifications.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Environments:
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