Skip to content

fix: set responses without content schemas to never or unknown, depending on status code #334

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

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Oct 19, 2020

fixes #333

@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #334 into master will increase coverage by 3.95%.
The diff coverage is 90.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   86.40%   90.35%   +3.95%     
==========================================
  Files           5        5              
  Lines         250      228      -22     
  Branches       88       79       -9     
==========================================
- Hits          216      206      -10     
+ Misses         22       15       -7     
+ Partials       12        7       -5     
Impacted Files Coverage Δ
src/index.ts 81.81% <60.00%> (-4.55%) ⬇️
src/v2.ts 83.72% <81.57%> (-2.95%) ⬇️
src/v3.ts 92.92% <92.63%> (+24.63%) ⬆️
src/utils.ts 97.77% <97.05%> (+0.34%) ⬆️
src/property-mapper.ts 84.21% <100.00%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3526df...faafeba. Read the comment docs.

@@ -32,7 +32,7 @@ export interface OpenAPI3Parameter {

export interface OpenAPI3ResponseObject {
description?: string;
content: {
content?: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

/**
* User not found
*/
'404': any
'404': unknown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 I’m OK with this change. Really, any undeclared type should be “dangerous” and I do like that this will surface more errors than any

@@ -630,4 +630,78 @@ describe("OpenAPI3 features", () => {
}`)
);
});

it("empty responses (#333)", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice unit test!

Currently the strategy has been to rely on unit tests as the real core of testing, but the full-schema “snapshots” are just ensuring that this actually works on real-world schemas. Glad you were able to figure it out!

The unit tests themselves could probably be better-organized; I’m not that picky. Open to feedback there. As long as they’re there (like the great ones you added), I’m happy!

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you

@drwpow drwpow merged commit 435ccd7 into openapi-ts:master Oct 20, 2020
@drwpow
Copy link
Contributor

drwpow commented Oct 20, 2020

@all-contributors please add @gr2m for code, test

@allcontributors
Copy link
Contributor

@drwpow

I've put up a pull request to add @gr2m! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set empty response type to never
2 participants