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

Simplify error using thiserror crate #7974

Closed
wants to merge 2 commits into from
Closed

Conversation

lewiszlw
Copy link
Member

Which issue does this PR close?

Simplify error using thiserror crate

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

It doesn't seem to have much benefits, but it adds a dependence

@lewiszlw
Copy link
Member Author

Better readability and maintainability, and thiserror is de facto standard for error handling.

@tustvold
Copy link
Contributor

de facto standard for error handling

The only crate that currently depends on thiserror is apache-avro which is currently optional and likely to be removed once supported upstream - apache/arrow-rs#4886.

I'm not against using thiserror, but it does seem a touch overkill for this use-case

@alamb
Copy link
Contributor

alamb commented Nov 2, 2023

Thanks again for this PR @lewiszlw

The reason we have a template for PRs is to avoid getting PRs getting stuck like this while the reviewers don't understand the rationale / purpose for the change.

Perhaps you can fill out this section

Rationale for this change

But as the other reviewers have said, it seems like all this PR does is add a new dependency without any corresponding gain in functionality. Perhaps we are missing something -- can you please let us know?

@alamb alamb marked this pull request as draft November 2, 2023 20:04
@alamb
Copy link
Contributor

alamb commented Nov 2, 2023

Marking as draft as this PR is no longer waiting on feedback

@lewiszlw
Copy link
Member Author

lewiszlw commented Nov 3, 2023

This pr just for simplifing code and improving readability. Many open source projects use thiserror for error handling. I think it's a good choice. If you have concern, feel free to close the pr.

@lewiszlw lewiszlw closed this Apr 8, 2024
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.

4 participants