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

Define jsonUnknown trait #180

Merged
merged 17 commits into from
Aug 13, 2024
Merged

Conversation

benoitlouy
Copy link
Contributor

@benoitlouy benoitlouy commented Aug 6, 2024

This PR is meant to replace #135

@CLAassistant
Copy link

CLAassistant commented Aug 6, 2024

CLA assistant check
All committers have signed the CLA.

@benoitlouy benoitlouy changed the title jsonunknown trait Define jsonUnknown trait Aug 6, 2024
@benoitlouy
Copy link
Contributor Author

I made the jsonUnknown trait member exclusive within a structure. I believe it makes sense only to allow a single member to collect unknown fields during json parsing.

@benoitlouy benoitlouy requested a review from Baccata August 9, 2024 13:54
@benoitlouy
Copy link
Contributor Author

benoitlouy commented Aug 9, 2024

Should we update the jsonUnknown trait to only make it applicable to members targeting a map with members of type Document?
If allowing the trait to be applied on Document, someone could assign something else than a Document object to that member.

Edit: I made the update. I think it makes sense and makes writing JSON encoder more straightforward


/// Retain unknown fields of a containing structure in this document member
@trait(
selector: "structure > member :test(> map > member > document)"
Copy link
Contributor

@Baccata Baccata Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to answer your question, I'd like for this to be eventually extended to allow for annotating union members, to allow for capturing unknown values.

As long as we can amend the selector to also cater to that use-case in the future, I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we want to extend this trait to support unions, we can update the selector to
:is(structure > member :test(> map > member > document), union > member :test(> document))
This will allow the trait to be applied to a union member targeting a Document.

@Baccata
Copy link
Contributor

Baccata commented Aug 13, 2024

@benoitlouy the code looks good to me. All that's left I think is adding documentation here to describe the expected behaviour associated to the trait when the protocols support it.

@Baccata
Copy link
Contributor

Baccata commented Aug 13, 2024

@lewisjkl I'm happy with this. Feel free to review/merge/release

@lewisjkl lewisjkl merged commit 5cc8125 into disneystreaming:main Aug 13, 2024
3 checks passed
Baccata pushed a commit to disneystreaming/smithy4s that referenced this pull request Aug 23, 2024
Implement support for `jsonUnknown` trait defined in disneystreaming/alloy#180
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.

5 participants