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

Initial consolidation of java & rust jsonschemas #56

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

marijncv
Copy link
Collaborator

@marijncv marijncv commented Dec 16, 2024

Summary & Motivation

Contributes to #44

This performs an initial step in the consolidation of the types generated on the jsonschema definition between the java & rust implementations of pipes. This pulls out the jsonschema definitions defined in the rust implementation onto a higher level, to be used as the source of truth for the generated types in the future

How I Tested These Changes

  • tests pass

Changelog

Ensure that an entry has been created in CHANGELOG.md outlining additions, deletions, and/or modifications.

See: keepachangelog.com

@marijncv marijncv self-assigned this Dec 16, 2024
@marijncv marijncv changed the title Consolidate java & rust jsonschemas Initial consolidation of java & rust jsonschemas Dec 17, 2024
@marijncv marijncv marked this pull request as ready for review December 17, 2024 08:57
@danielgafni
Copy link
Contributor

The original Java Pioes repo had an issue regarding this constructor.

Overall, I was hoping you could proceed with the schema changes for Rust without touching Java for now.

I imagined we would then reconcile Java with these changes once the schemas are stabilized.

Unless @GingerYouth wants to step in and do the necessary Java changes as part of this PR.

Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

I talked to @GingerYouth and he'll be able to do Java changes next week. So I suggest we make a separate PR for Java.

@GingerYouth
Copy link
Collaborator

As for exception class, I planned to switch to generated class, and move extra functionality to a wrapper. For metadata need to think and test. I can deal with these files next week, when I'll have access to my laptop.

@marijncv marijncv marked this pull request as draft December 17, 2024 12:43
@marijncv
Copy link
Collaborator Author

Thanks for the input @danielgafni & @GingerYouth! I reverted most of changes so that this PR only touches the rust side of things

@marijncv marijncv requested a review from danielgafni December 17, 2024 12:57
@marijncv marijncv marked this pull request as ready for review December 17, 2024 12:57
@cmpadden cmpadden dismissed danielgafni’s stale review December 17, 2024 14:39

Dismissing as changes are no longer targeting Java.

Copy link
Collaborator

@cmpadden cmpadden left a comment

Choose a reason for hiding this comment

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

Confident in approving with the Java changes removed; but this sets us up nicely to consolidate moving forward, thanks.

For some reason CI isn't triggering, but I ran fmt and test locally, and things passed. GitHub is having issues right now, so I'm hoping it's because of that.

image

@cmpadden cmpadden merged commit a9e15c8 into main Dec 17, 2024
@cmpadden cmpadden deleted the consolidate-jsonschema branch December 17, 2024 14:50
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