SCAL-338917 chore: resolve @version SDK placeholders automatically at release time - #676
sastaachar wants to merge 1 commit into
Conversation
Contributors have to name the SDK version a member is available from, but the next published version is not decided until the bump-version-and-pr workflow runs, so today they guess. Six SDK versions cited in src/ were never published: 1.19.1, 1.27.0, 1.30.2 and 1.34.0 already shipped to the docs site that way, and main currently carries 31 annotations for 1.53.0 and 14 for 1.54.0 — all of them wrong the moment a release is cut as a patch instead of a minor. Authors now write `@version SDK: <TBD> | ThoughtSpot Cloud: 26.10.0.cl` and the release bump substitutes the real SDK version, before docgen regenerates the typedoc JSON the docs site is built from, committed as part of the release PR. Only the SDK half may be <TBD>. The ThoughtSpot release is not derivable from this repo, so no automation can fill it in and the author must state it; the new validate-version-tags PR check enforces that while the author is still around to fix it. Because SDK and Cloud minors advance in lockstep (1.N -> 26.(N-43).0.cl), the release step also warns when the two disagree — a warning, not a failure, since back-dating to an earlier release is sometimes correct. check-versions runs on prepublishOnly so a release can never ship an unresolved placeholder in its public API docs. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new version placeholder resolution script (resolve-version-placeholders.js) along with its test suite and Jest configuration, integrating a workflow to automatically resolve <TBD> SDK version placeholders in JSDoc tags while validating ThoughtSpot release versions. Feedback on the changes includes addressing a validation bypass for empty or whitespace-only ThoughtSpot segments, refactoring the resolution script to fail-fast before writing partial changes to disk, and correcting a British English spelling in the test suite to adhere to the repository's American English style guide.
| if (thoughtSpotSegments.length === 0) { | ||
| problems.push(PROBLEM.MISSING_THOUGHTSPOT); | ||
| } |
There was a problem hiding this comment.
If a developer writes @version SDK: <TBD> | (with an empty or whitespace-only ThoughtSpot segment), thoughtSpotSegments.length will be 1 (containing [' ']). This bypasses the thoughtSpotSegments.length === 0 check, meaning PROBLEM.MISSING_THOUGHTSPOT is not added. As a result, the PR check (--lint) will pass, and during release, it will resolve to @version SDK: 1.53.0 | , which is malformed and violates the style guide.
We should update the check to also flag empty or whitespace-only segments as missing.
| if (thoughtSpotSegments.length === 0) { | |
| problems.push(PROBLEM.MISSING_THOUGHTSPOT); | |
| } | |
| if (thoughtSpotSegments.length === 0 || thoughtSpotSegments.every(seg => !seg.trim())) { | |
| problems.push(PROBLEM.MISSING_THOUGHTSPOT); | |
| } |
References
- Use
@version SDK: X.Y.Z | ThoughtSpot Cloud: A.B.C.cl— no colon after@versionitself.@versiontag values should have a space after the pipe:SDK: 1.0.0 | ThoughtSpot: 8.0.0.cl. (link)
| function runResolve(results, sdkVersion, dryRun) { | ||
| const resolvable = results | ||
| .map((result) => ({ | ||
| ...result, | ||
| hits: result.hits.filter((hit) => hit.problems.includes(PROBLEM.UNRESOLVED_SDK)), | ||
| })) | ||
| .filter((result) => result.hits.length > 0); | ||
|
|
||
| const total = resolvable.reduce((count, result) => count + result.hits.length, 0); | ||
| if (total === 0) { | ||
| console.log('No <TBD> SDK version placeholders to resolve.'); | ||
| } else { | ||
| console.log(`Resolving ${total} placeholder(s) → SDK: ${sdkVersion}${dryRun ? ' (dry run)' : ''}`); | ||
| resolvable.forEach(({ filePath, updated, hits }) => { | ||
| hits.forEach(({ line, after, warning }) => { | ||
| console.log(` ${path.relative(process.cwd(), filePath)}:${line} ${after}`); | ||
| if (warning) { | ||
| console.log(` ⚠ ${warning}`); | ||
| } | ||
| }); | ||
| if (!dryRun) { | ||
| fs.writeFileSync(filePath, updated, 'utf8'); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // A `<TBD>` the release cannot fill in must stop the release, not ship. | ||
| const unresolvable = reportProblems(results, [PROBLEM.TBD_THOUGHTSPOT]); | ||
| if (unresolvable > 0) { | ||
| console.error( | ||
| `✖ ${unresolvable} annotation(s) have a <TBD> ThoughtSpot release, which cannot be resolved automatically.`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Currently, runResolve writes resolved placeholders to disk before checking for unresolvable TBD_THOUGHTSPOT errors. If there are any unresolvable errors, the script will still modify some files on disk before exiting with a non-zero code.
To ensure transactional safety and avoid leaving the repository in a partially modified/dirty state, we should check for any unresolvable errors and fail-fast before writing any changes to disk.
function runResolve(results, sdkVersion, dryRun) {
// Check for unresolvable errors first to fail-fast and avoid dirty writes
const unresolvable = results.flatMap(({ hits }) => hits)
.filter((hit) => hit.problems.includes(PROBLEM.TBD_THOUGHTSPOT)).length;
if (unresolvable > 0) {
reportProblems(results, [PROBLEM.TBD_THOUGHTSPOT]);
console.error(
`✖ ${unresolvable} annotation(s) have a <TBD> ThoughtSpot release, which cannot be resolved automatically.`,
);
process.exit(1);
}
const resolvable = results
.map((result) => ({
...result,
hits: result.hits.filter((hit) => hit.problems.includes(PROBLEM.UNRESOLVED_SDK)),
}))
.filter((result) => result.hits.length > 0);
const total = resolvable.reduce((count, result) => count + result.hits.length, 0);
if (total === 0) {
console.log('No <TBD> SDK version placeholders to resolve.');
} else {
console.log(`Resolving ${total} placeholder(s) → SDK: ${sdkVersion}${dryRun ? ' (dry run)' : ''}`);
resolvable.forEach(({ filePath, updated, hits }) => {
hits.forEach(({ line, after, warning }) => {
console.log(` ${path.relative(process.cwd(), filePath)}:${line} ${after}`);
if (warning) {
console.log(` ⚠ ${warning}`);
}
});
if (!dryRun) {
fs.writeFileSync(filePath, updated, 'utf8');
}
});
}
}| expect(parseArgs([])).toEqual({ check: false, lint: false, dryRun: false }); | ||
| }); | ||
|
|
||
| test('recognises each mode flag', () => { |
There was a problem hiding this comment.
The test description uses the British English spelling "recognises". According to the repository style guide (Rule 95), all written work must be in American English (en-US) format. Please use "recognizes" instead.
| test('recognises each mode flag', () => { | |
| test('recognizes each mode flag', () => { |
References
- All written work must be in American English (en-US) format. Use American English spelling (e.g., 'color' not 'colour', 'behavior' not 'behaviour'). (link)
commit: |
This branch has not been deployed
1 participant
Jira: SCAL-338917
The problem
Every public member carries a
@versiontag naming the SDK version it is available from, and those comments are published to developers.thoughtspot.com. But the next published version isn't decided untilbump-version-and-prruns — so at merge time the author has no way to know it, and guesses.The guesses are wrong. Six SDK versions cited in
src/**were never published to npm:This got confirmed while the branch was being written:
maincarried 45 annotations promising 1.53.0 and 1.54.0 when v1.52.1 — a patch — was cut and published. All 45 now tell external developers a member is available from a version that does not exist.The fix
Authors write a placeholder for the SDK half, and the real ThoughtSpot release for the other:
Only the SDK half may be
<TBD>. The ThoughtSpot release isn't derivable from this repo, so no automation can fill it in — the author has to state it.One script,
scripts/resolve-version-placeholders.js, wired in at three points:resolve-versions— runs inbumpVersion.ymlright afternpm versionand beforedocgen, so the typedoc JSON the docs site is built from carries real versions.srcadded to that workflow'sgit add, so the rewrite is reviewable in thechore: releasePR.lint-versions— newvalidate-version-tagsjob inpr-checks.yml. Fails if<TBD>appears after the|, or if a<TBD>annotation names no ThoughtSpot release at all — caught while the author is still around to fix it.check-versions— added toprepublishOnly. A release can't ship an unresolved placeholder in its public API docs.Since SDK and Cloud minors advance in lockstep (
1.N→26.(N-43).0.cl), the release step also warns when the two disagree. Warning, not failure — back-dating to an earlier release is sometimes deliberate.Notes for review
<TBD>, so the hundreds of legacy@version SDK: 1.19.0annotations stay valid. Both gates pass clean on currentmain.src/**file is touched by this PR — it's tooling, workflows and docs only.jest.config.scripts.jsso build tooling stays out of the shippedsrc/coverage thresholds.npm testnow runs both projects.ts-embed.spec.ts→ "should trigger Navigate only after UpdateEmbedParams has settled") passes in isolation and is order-dependent/flaky, pre-existing, and unrelated — this PR changes nosrcfile.Follow-ups, deliberately not in this PR
main(31× 1.53.0, 14× 1.54.0) to<TBD>so the next release self-corrects. Happy to add it here if you'd rather it land together.jest.config.sdk.jstestPathIgnorePatternsdoesn't exclude/.claude/, so a localnpm testalso runs 242 specs from stale checkouts under.claude/worktrees/— 232 suites, 981s, and phantom failures. CI is unaffected.🤖 Generated with Claude Code