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

Add basic support for v14 #85

Merged
merged 6 commits into from
Nov 16, 2020
Merged

Add basic support for v14 #85

merged 6 commits into from
Nov 16, 2020

Conversation

rnestler
Copy link
Member

@rnestler rnestler commented Feb 4, 2020

So the goal is to have an implementation which can serialize and deserialize v0.13, v14 and mixed implementations which provides compatibility for both. This means we need to make fields optional which were removed in v14 or were not present in v0.13.

Checklist

  • Tests added if applicable
  • README updated if applicable
  • CHANGELOG updated if applicable
  • If a commit includes a breaking change, include the string [breaking-change] somewhere in the commit message

@dbrgn
Copy link
Collaborator

dbrgn commented Nov 8, 2020

Version 14 was now officially released! https://github.com/SpaceApi/schema/blob/master/CHANGELOG.md

@rnestler rnestler changed the title Support 0.14 Add basic support for v14 Nov 10, 2020
@rnestler rnestler requested a review from dbrgn November 10, 2020 17:19
@rnestler rnestler marked this pull request as ready for review November 10, 2020 17:28
@rnestler
Copy link
Member Author

@dbrgn This just adds the most minimal v14 support, by just adding the api_compatibility field and supporting both v0.13, v14 or a mixed serialization. I suggest we merge it anyways and add the other stuff as we go 🙂

@rnestler rnestler mentioned this pull request Nov 10, 2020
19 tasks
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Nice! I like the approach.

Could you add a test for the v14 constructor?

fn default() -> StatusBuilderVersion {
StatusBuilderVersion::V0_13
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does a default impl for the version make sense? I'd rather remove the Default derive from StatusBuilder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it would make constructing StatusBuilder a lot more annoying since one would need to specify all the fields (see new(), v0_13(), v14() currently line 273.
If we remove Default I'd also remove the new constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, true. But from a user perspective, as long as we use the constructor methods, it shouldn't be an issue, right?

@rnestler
Copy link
Member Author

Could you add a test for the v14 constructor?

Added a test for the v14 and mixed builder.

Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Let's merge this with Default support for now!

@rnestler rnestler merged commit 001cefb into master Nov 16, 2020
@rnestler rnestler deleted the support-0.14 branch November 16, 2020 16:32
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