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

Expose an option to reject unknown JSON members #123

Closed
wants to merge 1 commit into from

Conversation

negz
Copy link
Member

@negz negz commented Feb 21, 2024

Description of your changes

Closes #121

@phisco This is how I would wrap JSON options in our own, to avoid coupling ourselves to the upstream (and not yet stable) json-experiment implementation too strongly.

I'm opening as a draft though because after writing this it occurred to me that I'd prefer to make RejectUnknownMembers the default behavior for AsObject. My rationale:

  • I think it's the safest/least surprising in our use-case.
  • I believe it matches the default behavior of protojson.Marshal (i.e. AsStruct).
  • It's a breaking behavior change, but...
    • My hunch is it won't break anyone in practice yet.
    • We're still at v0.x and the first release, so breaking changes are acceptable when warranted.
  • We can always add an option in future if there's a demand to turn it off.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

@negz negz requested a review from phisco February 21, 2024 21:31
@phisco
Copy link
Collaborator

phisco commented Feb 21, 2024

Makes sense to me making it the default behavior, should we add options to all functions ending up in a AsObject being called?

@negz
Copy link
Member Author

negz commented Feb 21, 2024

Makes sense to me making it the default behavior, should we add options to all functions ending up in a AsObject being called?

I think if we're okay making this the default behavior we can defer adding options at all for the time being.

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.

2 participants