Skip to content
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

make status codes numeric #381

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Conversation

jankuca
Copy link
Contributor

@jankuca jankuca commented Nov 23, 2020

Closes #380

@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #381 (7336095) into main (e73e0d3) will increase coverage by 0.52%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   92.91%   93.43%   +0.52%     
==========================================
  Files           5        5              
  Lines         268      259       -9     
  Branches       90       84       -6     
==========================================
- Hits          249      242       -7     
  Misses         13       13              
+ Partials        6        4       -2     
Impacted Files Coverage Δ
src/index.ts 81.81% <50.00%> (ø)
src/property-mapper.ts 84.21% <50.00%> (ø)
src/v2.ts 84.61% <87.50%> (+0.89%) ⬆️
src/v3.ts 96.89% <93.54%> (+0.63%) ⬆️
src/utils.ts 100.00% <100.00%> (ø)

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 135a93b...7336095. Read the comment docs.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

great PR! @drwpow if you agree with the change, I think we should release this as a breaking change.

I'll test the change against my own work with @octokit to see if I run into any trouble, as I ran into the same problem you did

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

This changes works well for my usage in @octokit. Accessing the responses works the same using ...["responses"]["200"]. The only difference is that keyof paths["..."]["get"]["responses"] returns 200 | 404 instead of "200" | "404"

I say let's go ahead with it. I'm not even sure if I would consider the change a breaking change, but when in doubt I'd always rather go with a breaking change.

@drwpow
Copy link
Contributor

drwpow commented Nov 26, 2020

I'm not even sure if I would consider the change a breaking change, but when in doubt I'd always rather go with a breaking change.

Yeah I could go either way here. I think it technically is a breaking change, but it’s a breaking change on a newly-released feature. I agree with erring on the side of breaking change, and cutting 3.0 for this

@drwpow drwpow merged commit c3e40b4 into openapi-ts:main Nov 26, 2020
@drwpow
Copy link
Contributor

drwpow commented Nov 26, 2020

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

@allcontributors
Copy link
Contributor

@drwpow

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

@jankuca jankuca mentioned this pull request Nov 26, 2020
@drwpow
Copy link
Contributor

drwpow commented Nov 27, 2020

OK I tagged & released 3.0.0-rc.0 with these changes. If y’all have time to test the changes that’d be great! Overall I feel pretty confident in it, but just in case wanted to just kick the tires a bit before releasing 3.0.0.

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.

Numeric status codes
3 participants