Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive error handling system for the wreq Ruby binding by adding custom exception classes that map wreq errors to specific Ruby exception types. The changes improve error reporting by providing more granular exception types instead of generic runtime errors.
Key changes:
- Added new
error.rsmodule with custom exception classes and error conversion logic - Refactored proxy handling to use
Proxy::all()directly and leverage the new error handling system - Removed the generic
format_magnus_errorfunction in favor of the more specificwreq_error_to_magnusconverter - Removed unused
set_if_some_map_okmacro variant that is no longer needed
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/error.rs | New module defining custom exception classes (ConnectionError, BuilderError, etc.) and conversion logic from wreq errors to Magnus errors |
| src/lib.rs | Added error module declaration, updated imports, and initialized error exceptions in the init function |
| src/client.rs | Updated proxy parsing to use new error handler and simplified macro usage by preprocessing proxy values |
| src/macros.rs | Removed unused set_if_some_map_ok macro variant |
| lib/wreq_ruby/error.rb | Added Ruby-side error class definitions with comprehensive documentation and usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Error as MagnusError, RModule, Ruby, exception::ExceptionClass, prelude::*, value::Lazy, | ||
| }; | ||
|
|
||
| static WREQ: Lazy<RModule> = Lazy::new(|ruby| ruby.define_module(crate::RUBY_MODULE_NAME).unwrap()); |
There was a problem hiding this comment.
[nitpick] Potential issue: The WREQ module is being defined here with define_module, but the same module is already defined in lib.rs (line 43). Calling define_module multiple times should be safe in Ruby/Magnus (it returns the existing module if it already exists), but it's redundant. Consider passing the module as a parameter to include() or using find_class instead of define_module to avoid confusion.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| pub fn include(ruby: &Ruby) { | ||
| Lazy::force(&CONNECTION_ERROR, ruby); | ||
| Lazy::force(&CONNECTION_RESET_ERROR, ruby); | ||
| Lazy::force(&TLS_ERROR, ruby); | ||
| Lazy::force(&REQUEST_ERROR, ruby); | ||
| Lazy::force(&STATUS_ERROR, ruby); | ||
| Lazy::force(&REDIRECT_ERROR, ruby); | ||
| Lazy::force(&TIMEOUT_ERROR, ruby); | ||
| Lazy::force(&BODY_ERROR, ruby); | ||
| Lazy::force(&DECODING_ERROR, ruby); | ||
| Lazy::force(&BUILDER_ERROR, ruby); | ||
| } |
There was a problem hiding this comment.
[nitpick] The include function doesn't return a Result, unlike http::include and client::include. While the current implementation uses unwrap() in the lazy initialization (which could panic), consider returning Result<(), Error> for consistency with other module includes and to allow proper error propagation if initialization fails.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.