Skip to content

feat: add a generic error type #47

Merged
leerho merged 6 commits into
apache:mainfrom
tisonkun:error
Dec 30, 2025
Merged

feat: add a generic error type #47
leerho merged 6 commits into
apache:mainfrom
tisonkun:error

Conversation

@tisonkun

@tisonkun tisonkun commented Dec 29, 2025

Copy link
Copy Markdown
Member

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Comment thread datasketches/src/error.rs Outdated
/// # Errors
///
/// If k is less than 10, returns [`ErrorKind::InvalidArgument`].
pub fn try_new(k: u16) -> Result<Self, Error> {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Demonstrate how a fallible version can be made.

We may later apply the same to cdf/pmf/rank/quantile.

@Xuanwo Xuanwo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others LGTM

Comment thread datasketches/src/error.rs Outdated
Comment thread Cargo.toml
Comment thread datasketches/src/error.rs Outdated
kind: ErrorKind,
message: String,
context: Vec<(&'static str, String)>,
source: Option<anyhow::Error>,

@notfilippo notfilippo Dec 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't source just be a Box<dyn Error> + (Sync + Send if needed)?

@tisonkun tisonkun Dec 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the auto convesion (From/Into) will have some trouble. I ever try to replace anyhow::Error with box dyn Error + Send/Sync in OpenDAL, but it ruins the experience and I remember some compile failure.

However, we don't depend on source error now. So now, I tend to remove this feature as well as the anyhow dependency. When we need it, we can add it back and evaluate the solution at that moment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested a review from Xuanwo December 29, 2025 08:55

@PsiACE PsiACE left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ZENOTME ZENOTME left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tisonkun

Copy link
Copy Markdown
Member Author

cc @leerho This PR should be mergeable now.

@leerho leerho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tisonkun gave his Thumbs-up, which is good for me.

@leerho leerho merged commit f5143c3 into apache:main Dec 30, 2025
9 checks passed
@tisonkun tisonkun deleted the error branch December 30, 2025 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add general error type

6 participants