-
Notifications
You must be signed in to change notification settings - Fork 395
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: [577] Add yml support #588
feat: [577] Add yml support #588
Conversation
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.
Thanks for the contribution! Let's take one more pass at it, then land it.
While I understand why it would be cool for this to be supported without any extra steps, it will mean that all users have to pull in extra dependencies for something that they might not be using. For a library this can be easily solved by making the dependency an optional peer, but for CLIs that's not as easy. There's also the argument of "where does it stop?" What would jsonc, json5, and toml support? I wonder if it would be worth seeing if it might be better to encourage (and support, if not already) passing content in via pipes, and if that turns out to be straightforward go with that rather than this I.e |
@G-Rath That would be cleaner for sure. But because JSON Schemas can reference other schemas, and because we already support .yaml-backed references (https://github.com/APIDevTools/json-schema-ref-parser/tree/main/lib/parsers), I see supporting .yaml for root schemas as more about completeness. In theory if there was a way to make .yaml parsing/pre-processing pluggable all the way through, that would be nicer for sure! |
@bcherny fwiw while I've never used it like this myself, However, since |
Looks good! Please rebase to fix the merge conflict, then I'll merge this. |
Co-authored-by: Boris Cherny <[email protected]>
c4d8a21
to
ec2a12d
Compare
Thanks for the contribution! Will include this in the next release, next week. |
This PR is adding support for the yml files.
Fixes: #577