add cli directory support (using globs)#238
Conversation
|
FWIW this may also be better implemented in another library. Something that wraps this one. I'll let the maintainers decide what they'd like best |
bcherny
left a comment
There was a problem hiding this comment.
Looks great, thanks for the contribution! What do you think is a good way to unit test this?
|
Okay made changes; working on testing now |
|
Haha testing was a bit of a challenge but I got something working! Let me know what else you'd like for this new feature :) |
bcherny
left a comment
There was a problem hiding this comment.
Almost there -- sending this back to your queue.
|
Thank you for the review; will work on these recommendations soon and push changes! |
|
sorry it has been a little while, got swept up with final projects (yay for still being in college). |
6c60b63 to
ca9d682
Compare
|
The test suite is passing but I'm getting unhandelled promise rejection errors in the console. Trying to track it down now. |
1287712 to
a496af0
Compare
|
@bcherny hey! I'd like to try and complete this PR. I tried rebasing on master and I seem to have messed up the snapshots. Can you advise how I can fix this? |
a496af0 to
108a099
Compare
|
For those looking for directory access I've now published another npm module that implements such https://www.npmjs.com/package/compile-schemas-to-typescript (only api no cli right now) |
|
@Ethan-Arrowood Hey! Missed your comment. You want to re-generate the snapshots: git rebase origin/master
yarn test -u |
66bf06b to
7f2c9c8
Compare
|
Okay rebased and updated but there is some dep issue? Not sure whats wrong there |
There was a problem hiding this comment.
Getting closer, but this still needs a bit more cleanup and thinking about edge cases. Sending this back to your queue.
Also:
- It may be worth defensively running the testCLI.ts tests serially, so they don't step on each others' toes when concurrently interacting with the filesystem. Here's how to do that.
- Looking at the error output in CircleCI, the issue is that we're running tests on an old version of NodeJS:
yarn install v1.3.2
...
error @typescript-eslint/eslint-plugin@2.9.0: The engine "node" is incompatible with this module. Expected version "^8.10.0 || ^10.13.0 || >=11.10.1".
error Found incompatible module
...To fix that, I just bumped the version of NodeJS we use on CircleCI. If you rebase on the latest master, build should work. :)
e2bd33e to
8b42de2
Compare
|
Working on adding support for nested directory output now |
|
Fixing tests and adding the single output error catch now 😄 |
|
So I added the error check, but struggled to write a test for it. execSync isn't processing the error correctly for ava .throws() to catch |
|
@bcherny can you confirm what version of node this library is supposed to support? The package.json says 4.5 but I'm having issues with the @types/node version being so out of date. My latest commit bumps it to latest let me know if I should revert that and use old-style node code instead |
|
Bumping - what are my next steps to get this completed? |
bcherny
left a comment
There was a problem hiding this comment.
Looking good -- we're almost there. Sending this back to your queue for one last set of revisions, then we can merge this and publish out a beta for people to try it out.
|
Sorry for being MIA - will get to this asap (work has picked up significantly) |
|
Want to push the commit? Happy to pull it to help debug. |
|
Yep! Here it is: 5c1e875 |
|
@Ethan-Arrowood So you want to pull your |
bcherny
left a comment
There was a problem hiding this comment.
Looking good. Ready to merge? Anything else you want to change before we go for it?
|
I think its ready to go! |
|
@Ethan-Arrowood Mind rebasing? |
d6546a9 to
8fe487c
Compare
|
rebased :D |
bcherny
left a comment
There was a problem hiding this comment.
Final couple of comments (sorry, thought we were done). Also, want to do the honors of updating the README?
|
And yes I can update the README! |
bcherny
left a comment
There was a problem hiding this comment.
Thanks for the contribution @Ethan-Arrowood -- this is a long time coming!
|
Published 8.2.0. |
|
Wooo so happy this got merged! I'll be sure to contribute more in the future 😄 |
I'm working on adding cli support for glob patterns and directories of JSON files. This probably isn't complete just yet but I thought I'd share my work so I could get some initial feedback.
Ref #16
I need to familiarize myself with the library more, but it seems like there is need to add this to the programmatic side too (https://github.com/eweap/json-schema-to-typescript/blob/97f91e758317ef3ea5be3b9a1dfdd162051e3a48/src/index.ts#L68-L92). I'll try and use that as a reference