Replace process::exit() with Result propagation

[gcomte]

Summary

• CliInput::parse() now returns Result<Self, InputError> instead of calling process::exit() on invalid input
• Defaults::retrieve() and related methods now return Result instead of calling process::exit() on config errors
• Defaults::setup() returns Result instead of unwrap()-ing on config save (fixes Bump reqwest from 0.11.10 to 0.11.11 #6 too)
• Single error handling point in main()

Test plan

• cargo test passes
• Invalid input still produces clean error messages (no stack traces)

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling for invalid CLI input and configuration issues
  • Application now gracefully reports errors instead of terminating unexpectedly

• Refactor

  • Enhanced error propagation throughout the application for better diagnostics

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes refactor error handling from panic/exit patterns to explicit Result-based error propagation. cli_input.rs converts from From to TryFrom trait with fallible parsing helpers, defaults.rs changes infallible getters to fallible Result returns, and main.rs adds explicit error handling for CLI parsing failures.

Changes

Cohort / File(s)	Summary
Error Handling Refactoring src/cli_input.rs, src/defaults.rs	Converted infallible operations to fallible ones: impl From → impl TryFrom in cli_input.rs, parsing helpers now return Result<_, InputError>. In defaults.rs, public getters (get_default_amount, get_default_input_currency, get_default_output_currencies) and retrieve() now return Result<_, Box<dyn Error>> instead of panicking/exiting.
Error Propagation in Main src/main.rs	Added explicit match statement to handle CliInput::parse() results: Ok(input) continues normal flow, Err(e) prints error to stderr and exits with exitcode::USAGE.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A refactoring hop-worthy and true,
Where panics once fell, now Results come through!
From From to TryFrom, errors now spread,
No more abrupt exits—just graceful instead. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR objectives describe changes to error handling (CliInput, Defaults, main) but the only linked issue #6 requires bumping reqwest from 0.11.10 to 0.11.11, which is not reflected in the code changes provided.	Either update Cargo.toml to bump reqwest to v0.11.11 to satisfy issue #6, or remove the link to issue #6 if this PR only addresses error handling refactoring.
Out of Scope Changes check	⚠️ Warning	The PR changes CliInput parsing, Defaults retrieval, and main error handling to use Result types, which are in-scope. However, the PR includes no reqwest dependency bump despite being linked to issue #6 about that specific update.	Include the reqwest dependency bump in Cargo.toml if addressing issue #6, or clarify whether this PR should focus on error handling refactoring only and delink issue #6.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Replace process::exit() with Result propagation' accurately summarizes the main change: converting error handling from process termination to Result-based error propagation in CliInput, Defaults, and main.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/replace-process-exit-with-result

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/cli_input.rs (1)

90-100: ⚠️ Potential issue | 🟠 Major

Potential panic on empty string input.

If the user passes an empty string as the amount argument, amount.chars().last() returns None, and the .unwrap() on line 93 will panic. This contradicts the PR's goal of replacing panics/exits with clean error handling.

Suggested fix

             Some(mut amount) => {
+                if amount.is_empty() {
+                    return Err(InputError::new("Amount cannot be empty!"));
+                }
+
                 // check whether last character is an SI unit
                 let mut multiplier = 1.0;
                 let last_char = amount.chars().last().unwrap();

Alternatively, handle the None case directly:

                 // check whether last character is an SI unit
                 let mut multiplier = 1.0;
-                let last_char = amount.chars().last().unwrap();
+                let Some(last_char) = amount.chars().last() else {
+                    return Err(InputError::new("Amount cannot be empty!"));
+                };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli_input.rs` around lines 90 - 100, The code currently unwraps
amount.chars().last() which will panic if amount is empty; update the branch
handling Some(mut amount) in the parsing logic to first check for an empty
string (e.g., amount.is_empty()) or handle the Option from amount.chars().last()
instead of calling unwrap(); if empty, return or propagate a clean error (or
skip SI-prefix handling) rather than panicking, and then continue using
SiUnitPrefix::parse_from_str(&last_char.to_string()) and multiplier as before
once a valid last_char is available.

🧹 Nitpick comments (2)

src/cli_input.rs (1)

57-61: Remove deprecated Error::description() implementation.

The description() method has been deprecated since Rust 1.42.0. The Display implementation (lines 51-55) is sufficient for the Error trait.

Suggested change

-impl Error for InputError {
-    fn description(&self) -> &str {
-        &self.details
-    }
-}
+impl Error for InputError {}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli_input.rs` around lines 57 - 61, Remove the deprecated
Error::description implementation from the impl Error for InputError block:
delete the fn description(&self) -> &str { &self.details } method and leave
either an empty impl Error for InputError {} or remove the impl block entirely
if there are no other Error methods; keep the existing Display implementation
for InputError as the canonical error message provider.

src/defaults.rs (1)

69-74: Consider using Display formatting instead of Debug for the error.

Line 72 uses {e:?} (Debug format) which may produce verbose output with internal details. Using {e} (Display format) typically gives cleaner user-facing messages.

Suggested change

     fn setup(config: &HomeConfig) -> Result<(), Box<dyn Error>> {
         config
             .save_yaml(Self::load_defaults_template())
-            .map_err(|e| format!("Failed to save default config: {e:?}"))?;
+            .map_err(|e| format!("Failed to save default config: {e}"))?;
         Ok(())
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/defaults.rs` around lines 69 - 74, In setup (fn setup) the map_err uses
Debug formatting "{e:?}" for the save_yaml error; change the map_err closure to
use Display formatting "{e}" so the error message is cleaner for users (i.e.,
replace format!("Failed to save default config: {e:?}") with format!("Failed to
save default config: {e}") in the map_err on config.save_yaml).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/cli_input.rs`:
- Around line 90-100: The code currently unwraps amount.chars().last() which
will panic if amount is empty; update the branch handling Some(mut amount) in
the parsing logic to first check for an empty string (e.g., amount.is_empty())
or handle the Option from amount.chars().last() instead of calling unwrap(); if
empty, return or propagate a clean error (or skip SI-prefix handling) rather
than panicking, and then continue using
SiUnitPrefix::parse_from_str(&last_char.to_string()) and multiplier as before
once a valid last_char is available.

---

Nitpick comments:
In `@src/cli_input.rs`:
- Around line 57-61: Remove the deprecated Error::description implementation
from the impl Error for InputError block: delete the fn description(&self) ->
&str { &self.details } method and leave either an empty impl Error for
InputError {} or remove the impl block entirely if there are no other Error
methods; keep the existing Display implementation for InputError as the
canonical error message provider.

In `@src/defaults.rs`:
- Around line 69-74: In setup (fn setup) the map_err uses Debug formatting
"{e:?}" for the save_yaml error; change the map_err closure to use Display
formatting "{e}" so the error message is cleaner for users (i.e., replace
format!("Failed to save default config: {e:?}") with format!("Failed to save
default config: {e}") in the map_err on config.save_yaml).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3fe1ec34-2f1f-458e-a8d2-e14200ad90d9

📥 Commits

Reviewing files that changed from the base of the PR and between 045f586 and 69a1814.

📒 Files selected for processing (3)

• src/cli_input.rs
• src/defaults.rs
• src/main.rs