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

refactor: enhance error handling with custom ConfigError type #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codope
Copy link
Member

@codope codope commented Jul 9, 2024

  • Created ConfigError enum for more robust error handling
  • Updated ConfigParser trait and its implementations to use ConfigError
  • Modified HudiInternalConfig, HudiReadConfig, and HudiTableConfig to leverage ConfigError
  • Adjusted tests to align with new error handling

@codope codope changed the title feat(config): enhance error handling with custom ConfigError type refactor: enhance error handling with custom ConfigError type Jul 9, 2024
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.17%. Comparing base (c454088) to head (c01adc4).
Report is 20 commits behind head on main.

Files Patch % Lines
crates/core/src/config/mod.rs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
- Coverage   87.19%   87.17%   -0.02%     
==========================================
  Files          13       13              
  Lines         687      702      +15     
==========================================
+ Hits          599      612      +13     
- Misses         88       90       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pub enum ConfigError {
NotFound,
ParseError(String),
Other(String),
Copy link
Member

Choose a reason for hiding this comment

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

that's a nice improvement. I would suggest capture underlying error as source, like ParseError should capture std::ParseIntError, etc, and NotFound should capture which key (ConfigParser) it refers to.

On a bigger scope, we should definitely standardize error types throughout hudi-core and other hudi crates. I chose anyhow for fast iteration and uncover error handling paths first. So all errors come out from hudi are now anyhow::Error. I suggest replace anyhow dependency with well-defined custom error enums implemented with thiserror in the next release.

@xushiyan xushiyan linked an issue Jul 19, 2024 that may be closed by this pull request
@xushiyan xushiyan added this to the release-0.2.0 milestone Jul 19, 2024
@xushiyan xushiyan added feature rust Related to Rust codebase labels Jul 19, 2024
@xushiyan xushiyan modified the milestones: release-0.2.0, release-0.3.0 Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature rust Related to Rust codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Hudi error types across hudi-core
2 participants