-
-
Notifications
You must be signed in to change notification settings - Fork 515
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(biome_deserialize_derive): adding a rest attribute #2757
feat(biome_deserialize_derive): adding a rest attribute #2757
Conversation
- Added error when using unknown_fields = deny
6e9cb25
to
ae242f2
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.
This looks like a good first shot :)
We also need to update the derive macro documentation.
Have you tested this change with your use case?
To work with my use case, we need to add a catch all type that can implement |
Sorry I completely miss your reply.
I think a better solution could be of keeping the JSON tree around and to update the tree based on the changes made on the deserialized data. This could look like:
I think, for now, this represents too much work. We could keep the idea of the flatten attribute and provide generic structures to represent any data. We could have this kind of structure: enum AnyValue {
Null,
Bool(bool),
Number(TextNumber),
Str(Text),
Array(Vec<GenericValue>),
Map(HshMap<Text, GenericValue>),
} The disadvantage of this approach is that we deserialize data we don't care about. However, this should be enough to cover your use case for now. What do you think? |
Hmm, I was thinking it's okay to implement Ultimately though it's up to you! I have a PR that implements
We actually do have a limited version of the in place tree modification in the turborepo codebase. And ofc that would be doable with rowan/immutable trees, but agreed that this is out of scope. |
|
@Conaclos, any more feedback? I wrote up a little documentation on the attribute |
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 to me! Thanks!
@Conaclos, thanks so much! Do you think it would be possible to do a release for the crates? |
Sure. I'll have a look later today. |
I published |
Summary
We want the ability to put any unknown fields into a catch-all field, like serde's flatten attribute. This allows for parsing a package.json file while preserving the unknown fields.
Discussion here
This is a first draft, I'd love some guidance on the correct naming/API. Perhaps we could add a new option to the
unknown_fields
container attribute?Test Plan
Would love some guidance here!