-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[comparison] specTypeSchema/isSpecType as string-arg functions #1979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
felixweinberger
wants to merge
1
commit into
fweinberger/spec-type-predicates
Choose a base branch
from
fweinberger/spec-type-predicates-fn
base: fweinberger/spec-type-predicates
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: Several hunks in
docs/migration.mdanddocs/migration-SKILL.mdare pure Prettier reformatting unrelated to theisSpecType/specTypeSchemachange — this SdkErrorCode table re-padding, theitems: [/* ... */]code-block reflow at 382-389/397-402, the prose re-wraps at 140-141/441-442, and the Types/shared-imports table re-padding inmigration-SKILL.md. Since the PR's stated purpose is that the diff against #1887 "shows only the API-shape delta", it'd be worth running Prettier on the #1887 branch first so these whitespace hunks disappear from the comparison (you can't just drop them here — the format check would re-add them).Extended reasoning...
What this is
This PR is explicitly a comparison artifact: the description says it "targets the #1887 branch so the diff shows only the API-shape delta." Its value to reviewers is that scrolling the diff should reveal exactly what changes when you swap
isSpecType.X(v)/specTypeSchemas.XforisSpecType('X', v)/specTypeSchema('X')— and nothing else.However, the diff contains several hunks that are pure Prettier reformatting with zero semantic relation to the API-shape change:
docs/migration.md:719-734— the entireSdkErrorCodetable is re-padded because one row (InvalidResult, "Response result failed local schema validation") is wider than the previous max, so every other row's trailing-space padding shifts. No content changed.docs/migration.md:382-389and397-402—return { items: [/* ... */] };is reflowed to a 5-line block. This code sample is in the §"Custom (non-spec) methods" section and has nothing to do withisSpecType.docs/migration.md:140-141and441-442— two prose paragraphs (the auth-helpers paragraph and therequest()schema-parameter paragraph) are re-wrapped at a different column. Content identical.docs/migration-SKILL.md— the "Types / shared imports" table is re-padded to a much wider second column, and the §8 auth paragraph is re-wrapped.Step-by-step proof
Take the
SdkErrorCodetable hunk anchored on this line. Comparing old vs. new:NotConnectedrowTransport is not connected+ 18 trailing spaces + `InvalidResultrowEvery changed line in this hunk differs only in trailing-space count inside the table cell. Rendered Markdown is byte-for-byte identical. The same holds for the migration-SKILL.md table (the
shared/stdio.jsrow's long parenthetical forces the column wider, and Prettier re-pads every other row to match). None of these hunks mentionisSpecType,specTypeSchema,SpecTypeName, or anything else this PR is about.Why existing tooling doesn't prevent it
It's the opposite — the repo's Prettier enforcement causes it. The base branch (#1887) evidently left these files in a non-Prettier-normalized state (likely a row was added to each table / a phrase was edited without re-running the formatter on the whole file). When this PR touched the same files to update the
isSpecTypeexamples, format-on-save /pnpm formatnormalized the surrounding content too, and those normalizations show up in the inter-branch diff.Addressing the counter-argument
One could argue this isn't worth a comment because (a) the author can't simply drop these hunks — Prettier CI would fail or re-add them, (b) GitHub's
?w=1whitespace toggle hides the table re-padding and prose re-wraps, and (c) the actionable fix lives on #1887, not here.All three points are correct, and (a) in particular is why the suggested fix is not "drop these hunks from this PR" but rather "Prettier-clean the base #1887 branch and rebase." That is a one-command fix on #1887, after which this PR's diff collapses to exactly the API-shape delta it advertises. Point (b) mitigates but doesn't eliminate the noise:
?w=1hides the table padding and prose wraps, but theitems: [/* ... */]→ multiline reflow at 382-389/397-402 still appears as a content change even with whitespace hidden, since it inserts newlines between tokens. And (c) is precisely why this is filed as a nit rather than a blocking issue — it's process feedback about the comparison's readability, surfaced here because this is where the noise is visible.Impact and fix
No correctness impact whatsoever — rendered docs are identical. The only cost is reviewer attention: someone comparing the two API shapes has to mentally discard ~40 lines of table-padding diff and a couple of code-block reflows to find the ~15 lines that actually demonstrate the API difference. Given the PR exists specifically to make that comparison easy, tightening it up is worth a one-liner.
Fix: On the #1887 branch, run
pnpm format(or equivalent) overdocs/migration.mdanddocs/migration-SKILL.mdand commit. Then rebase this branch; the formatting hunks vanish and only theisSpecType/specTypeSchemaedits remain.