-
Notifications
You must be signed in to change notification settings - Fork 28
Bump wincode-derive to 0.4.0 #157
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
Conversation
|
Is #124 a breaking change or will pre-existing bounds continue to compile ? Thinking about semver and wanting to make sure this should be |
Pre-existing bounds should continue to compile. For existing |
steviez
left a comment
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.
Refreshed myself here, a few notes:
- Since we are in
0.x.y, no guarantee of stability for minor - If there is an API change, I believe the proper thing to do here is to bump the minor. It doesn't definitively state though, just this tip here when dealing with
0.x.ydevelopment
Looking at who is currently using wincode, it is:
- Anza owned repo's
gptman(rust-disk-partition-management/gptman#144)coreutils(uutils/coreutils#10134)
That being said, if we wanted to be very strict about this, a 0.4.0 release is justifiable.
Given that this crate already has some adoption outside of Anza and may see more, I think we should do "the right thing" and jump to 0.4.0. We got a lot of flack (rightfully so) for our abuse of semver in the SDK, so we should probably continue to be on our best behavior moving forward 😅
b56cf2a to
fa56742
Compare
|
Updated to I also had to remove the If both |
steviez
left a comment
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.
Can you please update the PR title as well to reflect the update ? Otherwise, LGTM
|
Going to hold off on merging this until #152 is merged so we can avoid minor version bump churn. |
yihau
left a comment
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.
nit: maybe could consider this: #158 so that we don't need to remove the windoe-derive version here https://github.com/anza-xyz/wincode/pull/157/changes#diff-4d133aae7d845b7fddba013210f841831b8b322b63624ba26a0e805b8eca2737L34 (also we will need to add it back later)
for breaking api version bump, in current situation, we will need to
A:
- have a PR to bump wincode-derive to next version and remove version field in wincode
- release wincode-derive
- have a PR to add version back to wincode, and here, I guess we must use the latest version
if we do patch, it will look like:
B:
- have a PR to bump wincode-derive to next version. no changes to wincode are required
- release wincode-derive
- have a PR to adapt wincode to use the new version
fa56742 to
6039226
Compare
|
|
|
Hmm, it looks like CI might be using the |
6039226 to
4d59dc4
Compare
|
Okay, restoring what we had previously, but pinning the version in |
This version includes the following bug fixes and features for
wincode-derive:SchemaReadref constraints #150Full commit diff