docs: correct circular reasoning about the message_* variant refs - #15
Merged
Conversation
PR #14 claimed six of the twelve base-pointing refs are correct because message_error, message_info and message_warning have no request variants to point at. That reasoning is circular: they have no request variants BECAUSE of this same defect. Variant need is propagated by scanning a schema for external refs, and that scan skips top-level composition keywords for exactly the reason the refs are not rewritten, so nothing ever marks those three as needing a variant. The missing files are a symptom, not independent evidence. Upstream's fix therefore does more than rewrite refs: python-sdk#83 creates fourteen new schemas — error_code, info_code, warning_code, message_error, message_info, message_warning and signed_amount, each in create and update form — taking the corpus from 145 files to 159. Every count in the README ending in 145 moves when it lands. Also corrects the upstream status recorded in #14. python-sdk#83 is not a companion fixing a SyntaxError that #35 triggers; it is the fix itself, carrying the same preprocess_schemas.py change plus the downstream codegen fix it requires plus regenerated models, and it supersedes #35. The earlier claim came from reading the first hunk of the diff, which lands in postprocess_models.py, and generalizing from it. The guard test's mechanism was already right and is unchanged — the os.Stat check will start matching the moment those variants exist, so the test reports message_create_request and message_update_request as newly affected. Only the comment explaining why they are excluded was wrong.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two corrections to what #14 recorded, both found by re-reading python-sdk#83 properly.
1. The count reasoning was circular
#14 said six of the twelve base-pointing refs are correct, because
message_error,message_infoandmessage_warninghave no request variants to point at.They have no request variants because of this same defect. Variant need is propagated by scanning a schema for external refs — and that scan skips top-level composition keywords, which is precisely why the refs are not rewritten. So nothing ever marks those three as needing a variant. The missing files are a symptom of the bug, not evidence that the bug does not apply there.
The behavioural counts are unchanged and still correct: 4 refs cause real harm today (
fulfillment_destinationcreate/update), 2 are wrong but inert (postal_address's variant is identical to its base). What changes is that the remaining six are not a clean bill of health — they are the bug hiding one layer down.2. The fix is larger than described
Because those variants are missing because of the defect, upstream's fix creates them. #83 adds fourteen new schemas —
error_code,info_code,warning_code,message_error,message_info,message_warning,signed_amount, each in create and update form.The corpus goes 145 → 159 files. Every count in the README ending in 145 moves when this lands. That is a materially bigger port than "rewrite six refs," and worth knowing before starting it.
3. The upstream status in #14 was wrong
#14 said #83 fixes a
SyntaxErrorin downstream codegen that #35's change triggers, and that "only #35 would affect us."#83 is the fix: it carries the same
preprocess_schemas.pychange as #35, plus thepostprocess_models.pyfix that change requires, plus regenerated models and a pipeline test. It supersedes the community PR.I had read the first 30 lines of the diff, which land in
postprocess_models.pybecause it sorts beforepreprocess_schemas.py, and generalized from that hunk to the whole PR.The guard test needed no change
Its mechanism was already right. The
os.Statcheck excludes themessage_*refs only while the variants do not exist — so the moment upstream creates them, the check starts matching and the test reportsmessage_create_requestandmessage_update_requestas newly affected. That failure is the intended signal.Only the comment explaining the exclusion was wrong, and it now says why the exclusion is temporary.
Verification
gofmtclean,go build,go vet, root tests and the conformance module all pass. No generated output changed — documentation and one test comment only.