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

Underlying type fix #79

Merged
merged 44 commits into from
Feb 21, 2024
Merged

Underlying type fix #79

merged 44 commits into from
Feb 21, 2024

Conversation

mfleader
Copy link
Member

@mfleader mfleader commented Feb 16, 2024

Changes introduced with this PR

'underlying type' == subtype

  • Fix (un)serialization of map data
  • Add discriminator_inlined field to OneOfSchema to allow users to declare the OneOfSchema's discriminator as a field in their subtype, and give it a default setting of false
  • Add function to validate whether or not the OneOfSchema's subtypes have been written correctly
  • Fix nil interfaceType bug where interfaceType is not set when a OneOfSchema is created without using a OneOfSchema constructor (NewOneOfIntSchema[T](), NewOneOfStringSchema[T]()).
  • Add unit tests to cover new behavior
  • Increase test coverage of error conditions

By contributing to this repository, I agree to the contribution guidelines.

@mfleader mfleader marked this pull request as ready for review February 16, 2024 16:23
@mfleader mfleader marked this pull request as draft February 16, 2024 16:51
@mfleader mfleader marked this pull request as ready for review February 16, 2024 17:04
webbnh

This comment was marked as resolved.

webbnh

This comment was marked as resolved.

@mfleader mfleader marked this pull request as draft February 19, 2024 19:38
@mfleader mfleader marked this pull request as ready for review February 20, 2024 16:35
@mfleader mfleader marked this pull request as draft February 20, 2024 16:35
@mfleader mfleader marked this pull request as ready for review February 20, 2024 16:37
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good once all of Webb's conversations are resolved. Remember to make sure the release is a pre-release. This is a potentially breaking change.

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I think this resolves all my comments (I'm not sure that you actually addressed my testing concerns, but the code that they referred to is all gone (I think), so I guess it's all good...).

I have two small comments which you can ignore if you like.

schema/oneof_test.go Outdated Show resolved Hide resolved
schema/oneof_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

This is good to go, unless there's a change to address my previous.

@mfleader mfleader merged commit 6e37a6a into main Feb 21, 2024
3 checks passed
@mfleader mfleader deleted the underlying-type-fix branch February 21, 2024 17:00
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.

4 participants