-
-
Notifications
You must be signed in to change notification settings - Fork 466
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
adds support for glob-style input spec paths and directory output paths #616
adds support for glob-style input spec paths and directory output paths #616
Conversation
const outputManifold = fs.readFileSync(path.join(__dirname, "generated", "manifold.ts"), "utf8"); | ||
expect(outputPetstore).toBe(expectedPetstore); | ||
expect(outputManifold).toBe(expectedManifold); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
let result = ""; | ||
for (const specPath of inputSpecPaths) { | ||
// append result returned for each spec | ||
result += await generateSchema(specPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever. I hadn’t thought about the stdout
case but this seems pretty handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait—we’re logging to stdout
elsewhere. Truth be told, I don’t know why we’re returning the result
here; it doesn’t seem to be helpful (and possibly wasting memory?). Either way, this is fine to leave in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I was initially confused as well about what the reason to return the result was. In user usage, the returned result doesn't matter, but I think it gets used in certain tests so it should be kept in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Yeah leave it in for now, and I can revisit that post-merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright!
Rebasing and updating expected schemas fixed the broken test, so I think this PR should be ready now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! Only reason why the tests are failing is the “expected” output isn’t up-to-date with latest main
; if you rebase then tests should pass. Great work!
Codecov Report
@@ Coverage Diff @@
## main #616 +/- ##
==========================================
+ Coverage 88.53% 90.25% +1.71%
==========================================
Files 9 10 +1
Lines 349 390 +41
Branches 123 141 +18
==========================================
+ Hits 309 352 +43
- Misses 37 38 +1
+ Partials 3 0 -3
Continue to review full report at Codecov.
|
@all-contributors please add @sharmarajdaksh for code |
I've put up a pull request to add @sharmarajdaksh! 🎉 |
This PR adds support for glob-style inputs and directory output paths for the openapi-typescript cli, as mentioned in #526.
With this, something like the following should be possible: