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 unknownFieldRetention trait #135

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

dhpiggott
Copy link
Contributor

No description provided.

.build()

val model =
Model.assembler.disableValidation
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why we have to disable validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I based this spec on one of the existing ones which had this. I'll see what happens if I enable it.

@dhpiggott dhpiggott force-pushed the define-unknown-field-retention-trait branch from 4417d9e to 739f418 Compare March 6, 2024 17:46
@dhpiggott dhpiggott force-pushed the define-unknown-field-retention-trait branch from 739f418 to a741ba6 Compare March 6, 2024 18:03
@trait(
selector: "structure > member :test(> document)"
)
structure unknownJsonFieldRetention {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @Baccata via Slack:

just unify the two traits as jsonUnkown. Document abides by json semantics, and we do implement the same semantics in both Document and jsoniter

@lewisjkl lewisjkl merged commit e270be0 into main Aug 13, 2024
2 of 3 checks passed
@dhpiggott dhpiggott deleted the define-unknown-field-retention-trait branch August 14, 2024 08:13
@dhpiggott
Copy link
Contributor Author

dhpiggott commented Aug 14, 2024

@lewisjkl GitHub told me you merged this but it also tells me that #180 replaced it. So wondering if you meant to merge this and if so how the two relate. I see:

lewisjkl merged commit e270be0 into main 14 hours ago

But then I see nothing on main 🤔 Bit confused!

@Baccata
Copy link
Contributor

Baccata commented Aug 14, 2024

Ben's PR forked from your branch, and had all the commits your branched contained. So merging Ben's PR into main implies merging this PR into main, as all the commits end up in main.

@dhpiggott
Copy link
Contributor Author

Aaaah, thanks! I should have looked at #180 more carefully, then I'd have realised.

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