-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: custom file formats in file content loader #12047
Conversation
🦋 Changeset detectedLatest commit: 1fb4e2d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
This PR is blocked because it contains a minor
changeset. A reviewer will merge this at the next release if approved.
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.
This is a great addition, thanks
5f8996e
to
d53d377
Compare
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, @rgodha24 , this looks like an amazingly helpful addition to the file()
loader!
I've added some comments and questions here about making a helpful changeset, which can also provide the basis for the documentation we'll need before this feature is released.
Mostly, my suggestions here are to give you an example of the kind of text/explanation I'd expect to see for this. I am the writer/editor, not the person who knows the feature, so it's up to you to help make this actually correct and meaningful! My suggestions are just to give you a model to use! Please do correct/improve as necessary!
Also, for @ascorbic - I've noticed this is for the main branch. Just checking that this will be for a minor 4.x release, and is not exclusively for the beta? This just determines exactly where/how we add to docs!
tysm for the review on the changeset! I implemented all of your suggestions.
I think this should be for the 5.0 release. Does that mean the PR should be merging into |
@rgodha24 yes, if it's just for 5.0 then it'll need to be rebased on |
5a59ecc
to
340778f
Compare
340778f
to
5f82af1
Compare
that was so painful but it should be good for review now! |
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.
This looks great. Thanks for sorting it out!
Changes
{"dogs": [{...}], "cats":[{...}]}
can be in one file by parsing a parse function like(text) => JSON.parse(text).cats
Testing
I added tests for a file loader with a yaml source, a toml source, and a nested json source. the toml source is also nested (that's the only way toml can be used in this scenario).
Docs
This definitely will need doc updates.
some parts that will be out of date:
I would definitely be willing to contribute docs for this feature
/cc @withastro/maintainers-docs