Improve description of responses with the same status code - #11891
Improve description of responses with the same status code#11891Jeremy Wharton (jewharton) wants to merge 1 commit into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed bug in status-code range parsing (ResponseIndex.entries()), and the union construction for grouped response types drops per-member documentation that the TypeKit can preserve.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates @typespec/http response processing so that when an operation has multiple responses sharing the same status code, the response description is derived by considering all variants rather than only the first one.
Changes:
- Refactors response collection to group processed response variants by status code and build a combined
HttpOperationResponseper group. - Updates description resolution to compare per-variant “single-response” descriptions (union/envelope docs) and otherwise fall back to
@returnsDoc/@errorsDocor RFC defaults. - Adds tests covering the new multi-response-per-status-code description rules.
File summaries
| File | Description |
|---|---|
| packages/http/src/responses.ts | Groups responses by status code and computes a shared description across all responses with that status code. |
| packages/http/test/response-descriptions.test.ts | Adds tests validating description selection when multiple responses share the same status code. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
ResponseIndex.entries() currently mis-parses status-code ranges (producing NaN start/end), which would break range-based responses at runtime.
Review details
Suppressed comments (1)
packages/http/src/responses.ts:166
ResponseIndex.entries()parses status-code ranges incorrectly: keys are stored as${start},${end}(see#indexKey), butentries()splits with":", which yieldsNaNstart/end and breaks range status codes.
} else if (indexKey.includes(",")) {
const [, start, end] = indexKey.split(":");
parsedStatusCodes = { start: Number(start), end: Number(end) };
} else {
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
d6745ba to
30582da
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two newly added tests assert incorrect default/derived descriptions for status code 200, and the new shared-description logic incorrectly ignores empty-string docs due to a truthiness check.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/http/test/response-descriptions.test.ts:136
- This test says it falls back to the default description, but the expected string is the 201 "created" message; for status code 200 the default in this suite is "The request has succeeded." (see the first test).
strictEqual(
op.responses[0].description,
"The request has succeeded and a new resource has been created as a result.",
);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
30582da to
f274909
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a user-visible behavior change in @typespec/http without an accompanying .chronus changelog entry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
f274909 to
885141e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The changeset entry needs to meet the repo’s feature-entry guidance, and the PR should clarify (or adjust) the additional behavioral change to HttpOperationResponse.type when multiple responses share a status code.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
885141e to
4dcf40b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a concrete behavioral regression risk in downstream consumers (notably the experimental typekit’s error detection) and also changes HttpOperationResponseContent object shape by always including body: undefined.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
4dcf40b to
3158265
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There is an unused local (tk) in packages/http/src/responses.ts that will fail lint/noUnusedLocals and should be removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
3158265 to
87210ff
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The .chronus entry appears misclassified as feature instead of fix, which would lead to an incorrect version bump per the repository’s Chronus changeKind definitions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
87210ff to
4645111
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the stated resolution rules, includes comprehensive new tests for the added branches, and ships an appropriate .chronus fix entry.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Timothee Guerin (timotheeguerin)
added
the
int:azure-specs
commit: |
|
All changed packages have been documented.
Show changes
|
An operation may have multiple responses with the same status code. Previously, only one response per status code was considered when determining the description for that status code. The resolution rules were as follows: 1. If the response was a member of a union, use the outermost union documentation comment if it exists. 2. If the response was an envelope, use the envelope's `@doc` if it exists. 3. If the response was an `@error` model, use the operation's `@errorsDoc` if it exists; otherwise, use the operation's `@returnsDoc` if it exists. 4. Otherwise, fall back to the default RFC 2616 description for the status code. Now, all responses sharing a status code are considered: 1. Compute the description each response would produce if it were the only response with that status code (its union comment, falling back to its envelope comment). 2. If every response sharing the status code produces the same description, then use that description. 3. Otherwise, if all such responses are `@error` models, use the operation's `@errorsDoc`; if none are, use the operation's `@returnsDoc`. 4. Otherwise, fall back to the default RFC 2616 description for the status code.
4645111 to
c45ee8d
Compare
microsoft-github-policy-service
Bot
added
the
emitter:openapi3
There was a problem hiding this comment.
🟡 Changes recommended
The .chronus entry should attribute this user-visible OpenAPI output change to @typespec/openapi3 as well (lockstep stable group), not only @typespec/http.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| packages: | ||
| - "@typespec/http" | ||
| --- |
An operation may have multiple responses with the same status code. Previously, only one response per status code was considered when determining the description for that status code. The resolution rules were as follows:
If the response was a member of a union, use the outermost union documentation comment if it exists.
If the response was an envelope, use the envelope's
@docif it exists.If the response was an
@errormodel, use the operation's@errorsDocif it exists; otherwise, use the operation's@returnsDocif it exists.Otherwise, fall back to the default RFC 2616 description for the status code.
Now, all responses sharing a status code are considered:
Compute the description each response would produce if it were the only response with that status code (its union comment, falling back to its envelope comment).
If every response sharing the status code produces the same description, then use that description.
Otherwise, if all such responses are
@errormodels, use the operation's@errorsDoc; if none are, use the operation's@returnsDoc.Otherwise, fall back to the default RFC 2616 description for the status code.