Skip to content
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

Merged
merged 4 commits into from Apr 21, 2024

Conversation

alexanderkrum
Copy link
Contributor

@alexanderkrum alexanderkrum commented Apr 10, 2024

This PR is adding support for the yml files.

Fixes: #577

@alexanderkrum alexanderkrum mentioned this pull request Apr 10, 2024
Copy link
Owner

@bcherny bcherny left a 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.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@G-Rath
Copy link
Contributor

G-Rath commented Apr 20, 2024

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 js-yaml provides a CLI for converting to json, so I wouldn't be surprised if right now you can do something like npx js-yaml path/to/schema.yml | npx json-schema-to-typescript -

@bcherny
Copy link
Owner

bcherny commented Apr 20, 2024

@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!

@G-Rath
Copy link
Contributor

G-Rath commented Apr 20, 2024

@bcherny fwiw while I've never used it like this myself, npx and co let you pass multiple packages which get installed in the same temp context so my understanding is you should be able to do something like npx -p js-yaml -p json-schema-to-typescript -- json-schema-to-typescript path/to/schema.yml

However, since js-yaml is already a dependency of json-schema-ref-parser, using it directly seems fine to me

@bcherny
Copy link
Owner

bcherny commented Apr 21, 2024

Looks good! Please rebase to fix the merge conflict, then I'll merge this.

@bcherny bcherny merged commit 9ec0c70 into bcherny:master Apr 21, 2024
18 checks passed
@bcherny
Copy link
Owner

bcherny commented Apr 21, 2024

Thanks for the contribution! Will include this in the next release, next week.

@alexanderkrum alexanderkrum deleted the feat/577-add-yml-support branch April 21, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add yaml support
3 participants