Skip to content

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.

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

@alexanderkrum alexanderkrum force-pushed the feat/577-add-yml-support branch from c4d8a21 to ec2a12d Compare April 21, 2024 05:20
@bcherny bcherny merged commit 9ec0c70 into bcherny:master Apr 21, 2024
@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