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

object_store: Migrate from snafu to thiserror #6266

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Aug 16, 2024

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 from snafu to thiserror, and then the final commit removes the now obsolete snafu 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.

@github-actions github-actions bot added the object-store Object Store Interface label Aug 16, 2024
@Turbo87
Copy link
Contributor Author

Turbo87 commented Aug 23, 2024

/cc @tustvold :)

@tustvold
Copy link
Contributor

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 😅

Also tagging @Xuanwo and @alamb who may have other thoughts

@Turbo87
Copy link
Contributor Author

Turbo87 commented Aug 23, 2024

the next breaking release will not be for another 3 months

no worries :)

as we can't merge it to master for another 2 months

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

@tustvold
Copy link
Contributor

any reason for not being able to merge before then

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

@alamb
Copy link
Contributor

alamb commented Sep 18, 2024

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

object_store/src/delimited.rs Outdated Show resolved Hide resolved
@Jefffrey Jefffrey added the next-major-release the PR has API changes and it waiting on the next major version label Sep 27, 2024
@findepi
Copy link
Member

findepi commented Sep 30, 2024

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?

@tustvold
Copy link
Contributor

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

@Turbo87
Copy link
Contributor Author

Turbo87 commented Sep 30, 2024

Would you consider moving eg first ~4 commits to a separate one?

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 Cargo.toml file. it's probably easiest to review this commit-by-commit. since the changes are indeed pretty mechanical this should get quicker after reviewing the first few commits.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Sep 30, 2024

FYI I've just rebased the branch to fix the merge conflict and integrate #6266 (comment)

@Turbo87
Copy link
Contributor Author

Turbo87 commented Oct 3, 2024

rebased again to fix merge conflicts caused by #6453

@jondot
Copy link

jondot commented Oct 30, 2024

This is great! I've been looking for this to optimize builds of Loco (https://github.com/loco-rs/loco)

@Turbo87
Copy link
Contributor Author

Turbo87 commented Nov 6, 2024

rebased once more and updated to thiserror v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-major-release the PR has API changes and it waiting on the next major version object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants