You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Turbo87 (via GitHub)" <gi...@apache.org> on 2024/08/16 20:12:38 UTC
[PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Turbo87 opened a new pull request, #6266:
URL: https://github.com/apache/arrow-rs/pull/6266
# Which issue does this PR close?
https://github.com/apache/arrow-rs/issues/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.
# 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551237838
tustvold beat me to it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "Turbo87 (via GitHub)" <gi...@apache.org>.
Turbo87 commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2458918807
rebased once more and updated to thiserror v2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "progval (via GitHub)" <gi...@apache.org>.
progval commented on code in PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#discussion_r1771827398
##########
object_store/src/delimited.rs:
##########
@@ -125,8 +124,12 @@ impl LineDelimiter {
/// Returns `true` if there is no remaining data to be read
fn finish(&mut self) -> Result<bool> {
if !self.remainder.is_empty() {
- ensure!(!self.is_quote, UnterminatedStringSnafu);
- ensure!(!self.is_quote, TrailingEscapeSnafu);
+ if self.is_quote {
+ return Err(Error::UnterminatedString.into());
+ }
+ if self.is_escape {
+ return Err(Error::TrailingEscape.into());
+ }
Review Comment:
```suggestion
if self.is_quote {
Err(Error::UnterminatedString)?;
}
if self.is_escape {
Err(Error::TrailingEscape)?;
}
```
Looks a bit simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "Turbo87 (via GitHub)" <gi...@apache.org>.
Turbo87 commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2306484388
/cc @tustvold :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "Turbo87 (via GitHub)" <gi...@apache.org>.
Turbo87 commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2306649422
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2306619931
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 :sweat_smile:
Also tagging @Xuanwo and @alamb who may have other thoughts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2359322599
I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see https://github.com/apache/arrow-rs/issues/6418 for more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551234035
> I think enough time has been passed to cut another major release
I think we we would want to coordinate this with the arrow major release, as otherwise everything will end up lagging.
I'm also not sure we really have enough functionality to warrant making a breaking release - see discussion on https://github.com/apache/arrow-rs/issues/6596
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551236519
> I think enough time has been passed to cut another major release. Before we send @Turbo87 off to fix the conflicts AGAIN, @alamb do you agree that we could merge this? Since the PR is a purely mechanical translation, the size is totally fine.
I think the as yet unresolved question is what the next object_store release will be -- breaking or non breaking:
- https://github.com/apache/arrow-rs/issues/6596
If it will be breaking we can merge this PR, if not, we should wait
Let's figure out what the next release will be on https://github.com/apache/arrow-rs/issues/6596
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "Turbo87 (via GitHub)" <gi...@apache.org>.
Turbo87 commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551536165
I have to admit that it's getting a bit frustrating that this PR has been open for 4 months and now it's looking like it gets to stay open for at least another 2 months...
Same for https://github.com/apache/arrow-rs/pull/6636. It sounds like the argument is that there are not enough breaking changes gathered, but I'm wondering what the threshold is since I kept maintaining these two breaking change PRs for a while now 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "Turbo87 (via GitHub)" <gi...@apache.org>.
Turbo87 commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2568259254
rebased once more. I guess this is ready to be merged now? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "jondot (via GitHub)" <gi...@apache.org>.
jondot commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2446994586
This is great! I've been looking for this to optimize builds of Loco (https://github.com/loco-rs/loco)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551867129
> Same for https://github.com/apache/arrow-rs/pull/6636. It sounds like the argument is that there are not enough breaking changes gathered,
I apologise that we've ended up in this position, and thank you for your patience. We've had a fair amount of push back from the community about the rate of breaking changes and so are trying hard to keep them down. Just under half of the install base still hasn't made it to 0.11 yet... - https://crates.io/crates/object_store
I've proposed that the next release after the one next week be a breaking release, and so we'd be able to get this merged to main in a week or so - https://github.com/apache/arrow-rs/issues/6596#issuecomment-2551864541
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2306659491
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2551205270
I think enough time has been passed to cut another major release. Before we send @Turbo87 off to fix the conflicts AGAIN, @alamb do you agree that we could merge this? Since the PR is a purely mechanical translation, the size is totally fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2568456881
🔫 🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] object_store: Migrate from `snafu` to `thiserror` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6266:
URL: https://github.com/apache/arrow-rs/pull/6266#issuecomment-2568457259
Thanks again for for sticking with this @Turbo87
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org