-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: replace anyhow #18
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request successfully refactors the codebase to replace the anyhow error handling crate with custom, strongly-typed error enums (ConfigError and ProviderError). The refactoring improves type safety and makes error handling more explicit throughout the application.
Changes:
- Introduced
ConfigErrorenum for configuration-related errors with specific variants for IO, YAML parsing, unknown providers, and unknown DNS types - Introduced
ProviderErrorenum to unify error handling across all DNS provider operations, wrapping provider-specific errors and common failure cases - Updated all provider implementations (Hetzner, Nitrado, Netcup) to use the new error types with consistent error mapping patterns
- Improved CLI error reporting with specific error variants for argument validation
- Removed the
anyhowdependency from the project
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Removed anyhow dependency |
| Cargo.lock | Updated dependency lock file to reflect anyhow removal |
| src/config.rs | Added ConfigError enum and updated all configuration functions to return Result<T, ConfigError> |
| src/lib.rs | Exported ConfigError for external use |
| src/main.rs | Updated Error enum to use ConfigError instead of anyhow::Error |
| src/cli/get.rs | Added specific error variants for argument validation and updated to use ProviderError |
| src/cli/generate_config.rs | Updated to use ConfigError instead of anyhow::Error |
| src/provider.rs | Updated Provider trait methods to return Result<T, ProviderError> |
| src/provider/error.rs | Added new ProviderError enum to unify provider error handling |
| src/provider/hetzner.rs | Updated to use ProviderError with explicit error mapping |
| src/provider/nitrado.rs | Updated to use ProviderError with explicit error mapping |
| src/provider/netcup.rs | Updated to use ProviderError with explicit error mapping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/provider/hetzner.rs:106
- This
expect()call can panic if the API key contains invalid characters for HTTP headers. SinceProviderError::InvalidApiKeyexists (defined in error.rs line 33), this should return that error instead of panicking. Consider using.parse().map_err(|_| ProviderError::InvalidApiKey)?or adding a helper method that validates the API key and returns a proper error.
headers.insert(
"Auth-API-Token",
self.provider_config.api_key.parse().expect("Invalid Hetzner API key: contains characters that are not allowed in HTTP headers"),
src/provider/nitrado.rs:68
- This
unwrap()call can panic if the formatted Bearer token contains invalid characters for HTTP headers. SinceProviderError::InvalidApiKeyexists (defined in error.rs line 32-33), this should return that error instead of panicking. Consider using.parse().map_err(|_| ProviderError::InvalidApiKey)?
headers.insert(
"Authorization",
format!("Bearer {}", self.provider_config.api_key)
.parse()
.unwrap(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kitt3120
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALready looking very good, just some quick fixes for you
| #[error("Cannot determine DNS config type for file: {0}")] | ||
| UnknownDnsType(String), | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if it makes sense to use individual Error enums for the different methods below instead of having them all return the same general ConfigError enum? Or does every method below actually need all of the enum values? Then using the same ConfigError enum for all of them makes sense. Otherwise, define specific error enums per method.
…s into chore/remove-anyhow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kitt3120
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored some code.
There is still 1 discussion open we should resolve before merging. Also, there are some clippy warnings left. Please run cargo clippy and see if you can fix them.
This pull request refactors error handling throughout the codebase to replace the use of the generic
anyhow::Errorwith custom, strongly-typed error enums for configuration and provider-related operations. This makes error handling more precise and improves error reporting. The most significant changes include the introduction ofConfigErrorandProviderErrortypes, their integration into the main application logic, and the propagation of these error types through the CLI and provider interfaces.Error Handling Refactor:
ConfigErrorenum insrc/config.rsto handle configuration loading and parsing errors, replacinganyhow::Errorthroughout the configuration code. All relevant functions now returnResult<T, ConfigError>and propagate this error type. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]ProviderErrorenum insrc/provider/error.rsthat unifies errors from all DNS providers and is now used as the error type for theProvidertrait methods. All provider implementations and trait signatures have been updated to use this error type instead ofanyhow::Error. [1] [2] [3] [4] [5] [6]CLI and Application Logic Adjustments:
ConfigErrorandProviderErrortypes, improving the specificity of error messages shown to users. Added new error variants for argument validation insrc/cli/get.rs. [1] [2] [3] [4] [5]src/main.rsto integrate the new error types, including a small change to boxRuntimeErrorfor consistency. [1] [2]Dependency and Code Clean-up:
anyhowinCargo.tomland all code files, as it is no longer needed after migrating to custom error types. [1] [2] [3]These changes collectively improve the robustness and clarity of error handling across the application, making it easier to diagnose and handle specific error cases.