Skip to content

fix: schema with properties and oneOf properties #491

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 2, 2021
Merged

fix: schema with properties and oneOf properties #491

merged 7 commits into from
Mar 2, 2021

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Mar 2, 2021 •
edited
Loading

@drwpow
Copy link
Contributor

drwpow commented Mar 2, 2021

@gr2m This looks like it’s WIP? Happy to review whenever; just tag me!

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

Yea it’s WIP, just added a failing test so far. I’ll let you know ones it’s ready

@gr2m gr2m force-pushed the schema_with_properties_and_anyOf branch from 814c96b to 14b7de8 Compare March 2, 2021 19:56
@codecov
Copy link

codecov bot commented Mar 2, 2021 •
edited
Loading

Codecov Report

Merging #491 (993dc22) into main (72ac95b) will increase coverage by 1.28%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
+ Coverage   78.52%   79.80%   +1.28%     
==========================================
  Files           9        9              
  Lines         298      312      +14     
  Branches       92       98       +6     
==========================================
+ Hits          234      249      +15     
+ Misses         64       63       -1
Impacted Files Coverage Δ
src/transform/index.ts 26.92% <50.00%> (+0.50%) ⬆️
src/transform/operation.ts 96.42% <92.85%> (+0.97%) ⬆️
src/transform/responses.ts 86.84% <100.00%> (+4.08%) ⬆️
src/transform/schema.ts 98.38% <100.00%> (+0.11%) ⬆️
src/utils.ts 90.56% <100.00%> (-0.67%) ⬇️

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 494fa56...993dc22. Read the comment docs.

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

@drwpow this is ready for review. In the first two commits I only update the OpenAPI spec from GitHub and our generated github.ts to bump the test baseline to latest. The 3rd commit is the tests I introduced, the 4th commit is the code changes. And finally the 5th commit shows that this change fixes the problem that came up in octokit/types.ts#274

@gr2m gr2m requested a review from drwpow March 2, 2021 19:58
@gr2m gr2m changed the title 🚧 fix: schema with properties and oneOf properties fix: schema with properties and oneOf properties Mar 2, 2021
@gr2m gr2m mentioned this pull request Mar 2, 2021
Closed
@drwpow
Copy link
Contributor

drwpow commented Mar 2, 2021

I only update the OpenAPI spec from GitHub

That works for me! I’d like for the specs/* folders to be real-world schemas we’re testing against. In the past, I’ve only been against contributors adding spec test-like additions to specs/*, because the whole point is to update these as the specs evolve, and we don’t want their changes to be blown away. But I’d love to keep specs/* updated so we always ensure we’re generating types correctly.

drwpow
drwpow reviewed Mar 2, 2021
View reviewed changes
src/transform/schema.ts Outdated
@@ -105,9 +111,12 @@ export function transformSchemaObj(node: any): string {

output += tsIntersectionOf([
...(node.allOf ? (node.allOf as any[]).map(transformSchemaObj) : []), // append allOf first
...(node.anyOf ? [transformAnyOf(node.anyOf)] : []), // append allOf first
...(node.oneOf ? [transformOneOf(node.oneOf)] : []), // append allOf first
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

drwpow
drwpow reviewed Mar 2, 2021
View reviewed changes
src/transform/schema.ts
@@ -77,8 +77,14 @@ export function transformSchemaObj(node: any): string {
break;
}
case "object": {
const isAnyOfOrOneOfOrAllOf = "anyOf" in node || "oneOf" in node || "allOf" in node;
Copy link
Contributor

Choose a reason for hiding this comment

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

😄 this logic has always broken my brain. This is clearer! Thanks

drwpow
drwpow reviewed Mar 2, 2021
View reviewed changes
tests/schema.test.ts
}>)) & ({
"a"?: string;

})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test 👍🏻

gr2m
gr2m commented Mar 2, 2021
View reviewed changes
gr2m
gr2m commented Mar 2, 2021
View reviewed changes
@gr2m gr2m merged commit 1638186 into main Mar 2, 2021
@gr2m gr2m deleted the schema_with_properties_and_anyOf branch March 2, 2021 22:11
@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

@drwpow could you cut a new release please?

@drwpow
Copy link
Contributor

drwpow commented Mar 2, 2021

Sure!

@drwpow
Copy link
Contributor

drwpow commented Mar 2, 2021

3.0.3 released

@drwpow drwpow mentioned this pull request Mar 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drwpow drwpow drwpow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gr2m @drwpow