-
Notifications
You must be signed in to change notification settings - Fork 802
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
object_store: Migrate from snafu
to thiserror
#6266
base: master
Are you sure you want to change the base?
Conversation
/cc @tustvold :) |
HI @Turbo87, thank you for this, it looks pretty spot on. Unfortunately this ended up missing the 0.11 release, the RC was cut on the 12th August, and the next breaking release will not be for another 3 months... I'm not really sure what the best way to proceed with this is, as we can't merge it to master for another 2 months. One option might be to keep this PR as a draft until then, but I appreciate that's likely a longer time commitment than you might have reasonably intuited when you picked this up 😅 |
no worries :)
any reason for not being able to merge before then? I would guess potential bug fixes could be branched off before the merge of this PR, but I'm not familiar with the release process here :D |
Non-backport releases, including minor releases, are made from master. There have been some discussions in the past about alternative approaches, but they're not without their own trade-offs. #5907 |
e01422b
to
90a3da1
Compare
I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more |
Smaller PRs are faster to merge, because the review time is more than linear. @Turbo87 your PR has awesome commit-by-commit structure. Would you consider moving eg first ~4 commits to a separate one? |
This PR is probably fine as is, it's largely mechanical, it is actually just waiting on us to start preparation for the next breaking release |
we can do that, but I'm not sure how valuable that would actually be since we'd then end up in a situation where we have both dependencies in the |
90a3da1
to
9b987d5
Compare
FYI I've just rebased the branch to fix the merge conflict and integrate #6266 (comment) |
9b987d5
to
bfe4165
Compare
rebased again to fix merge conflicts caused by #6453 |
bfe4165
to
6c32214
Compare
This is great! I've been looking for this to optimize builds of Loco (https://github.com/loco-rs/loco) |
6c32214
to
56947d0
Compare
56947d0
to
084744e
Compare
rebased once more and updated to thiserror v2 |
084744e
to
d49c00b
Compare
Which issue does this PR close?
#6166, though only partially, since the PR is only targeting the
object_store
crate.Rationale for this change
The rationale is explained in the linked issue :)
What changes are included in this PR?
The first commit in the PR adds a dependency on
thiserror
, the following commits incrementally port all error enums fromsnafu
tothiserror
, and then the final commit removes the now obsoletesnafu
dependency.Note that this PR also includes #6265 to avoid merge conflicts from the bugfix.
Are there any user-facing changes?
As the linked issue mentions, some of the
snafu
type may leak to the outside and this PR will remove them so this should probably be treated as a breaking change, even though these types have been considered private API before.