-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: support for sourceless nodes. #1386
Conversation
Thanks for the pull request. Please add tests and also fix the todo. |
@domoritz, for now, I don't have any spare time to to these tests, are you up to do it? They seem pretty small tests. Furthermore, the |
I'm starting to teach on Tuesday so I unfortunately don't have cycles to write tests for you. |
#/reference/
usage.#/reference/
usage.
Thanks for adding the tests. Ping me when they are passing and I will review the pull request. |
@domoritz, I think that was it! Got my new notebook yesterday and now i can code while I'm at philosophy classes 😁. The only thing remaining is this
ts-json-schema-generator/src/Utils/nodeKey.ts Lines 38 to 40 in 9e022fa
I'd would let it here just to our future self keep an eye on it. |
@domoritz, Can this be released soon? |
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.
Thank you for the pull request. I do really appreciate the time you put into making these changes and sending the pull request. Looking over the changes, I think this pull request does too many things, though.
In general, I want to avoid introducing new options especially when it's things that can easily be done by pos-processing the JSON or that are for a specific use case only. So in this case I don't think I want to include the change to make references optional. They are needed according to json schema spec, no?
What are sourceless types? If this is something common, I'd be happy to include the change as a separate pull request.
I'm happy to be convinced otherwise but I try to think about the future of the project and want to be thoughtful when adding features.
I'm calling sourceless nodes any |
About the And because of the above thread (any many others sub discussions spread out there), a lot of json schema related libraries are have different behaviours for This option is just another related feature that is disabled by default, and it helps in these cases without having to maintain a fork of this project with their required changes, as it currently this project does not allow all nescessary customizations to make it work. |
I agree with you that this PR has two unrelated to each other changes. I could split them into two small PRs, but that would lead to me just copying code over branches... |
I'm not convinced that I want to support optional references at this point. Can you remove it from this pull request? The sourceless node support makes sense to me, though. |
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
This pull request introduces 1 alert when merging 3010834 into f7e9cb5 - view on LGTM.com new alerts:
|
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.
anything else?
#/reference/
usage.
@domoritz, sorry for pinging, but this PR would help me so much :) |
Thanks for the ping. It's in my queue to review but I have a lot on my plate right now. I'd recommend publishing your fork in your personal namespace to get unblocked. |
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.
Looks good. Just one comment and then we can merge. Thanks for all the work on this!
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
Hey! I've been using your project to help with json schema generation for my recent
kitajs
project. Its being great, but ive encountered some problems with my specific scenarios.This PR is what I've been fixing along the time. A shortly brief:
typeChecker.typeToTypeNode
, which creates nodes without a valid source code and-1
locations.#/reference/
prefix, so I added another option too.Version
Published prerelease version:
v1.2.0-next.1
Changelog
🎉 This release contains work from new contributors! 🎉
Thanks for all your work!
❤️ Arthur Fiorette (@arthurfiorette)
❤️ Sean Keenan (@sean9keenan)
🚀 Enhancement
🐛 Bug Fix
🔩 Dependency Updates
Authors: 4