-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
Version 14 was now officially released! https://github.com/SpaceApi/schema/blob/master/CHANGELOG.md |
78d0245
to
070fbf5
Compare
[breaking-change] The `api` field is now an `Option`.
7480e9e
to
30c3a65
Compare
@dbrgn This just adds the most minimal v14 support, by just adding the |
There was a problem hiding this 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 | ||
} | ||
} |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Added a test for the |
79d1d61
to
459bc78
Compare
There was a problem hiding this 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!
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
[breaking-change]
somewhere in the commit message