Skip to content

Commit ad49ba3

Browse files
subyganCleanCut
andauthored
Api 159/better error handling in the client (#380)
example, <img width="1108" height="73" alt="Screenshot 2026-03-25 at 12 19 57 AM" src="https://github.com/user-attachments/assets/905010c2-9e6f-4517-a1ee-cb59131ac141" /> this adds a unified cli_error.rs handler to convert OxenError into user readable error. so far we've been abusing oxenError::basic_str() and bespoke eprintln!() at various layers. this should allow us to bubble up the actual error message and handle it at a higher level. --------- Co-authored-by: Nathan Stocks <nathan.s@oxen.ai>
1 parent e440014 commit ad49ba3

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

55 files changed

+343
-698
lines changed

.claude/CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ oxen push origin main # Push to remote
105105
- When changing something that is documented in nearby code, or appears in any markdown files in the repository, update the affected documentation.
106106
- When prompted to always do something a certain way in general, add an entry to this section of the CLAUDE.md file.
107107
- When calling `get_staged_db_manager`, follow the doc comment on that function: drop the returned `StagedDBManager` as soon as possible (via a block scope or explicit `drop()`) to avoid holding the shared database handle longer than necessary.
108+
- When altering the `OxenError` enum, consider whether a hint needs to be added or updated in the `hint` method.
108109
- After changing any Rust code, verify that tests pass with the `bin/test-rust` script (not `cargo`). The script is documented in a comment at the top of its file.
109110

110111
# Testing Rules

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/cli/src/cli_error.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
use colored::Colorize;
2+
use liboxen::error::OxenError;
3+
4+
/// Print an OxenError to stderr with colored prefix and optional hints.
5+
pub fn print_error(err: &OxenError) {
6+
eprintln!("{} {}", "Error:".red().bold(), err);
7+
8+
if let Some(hint) = err.hint() {
9+
eprintln!(" {} {}", "hint:".cyan().bold(), hint);
10+
}
11+
}

crates/cli/src/cmd/add.rs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,7 @@ impl RunCmd for AddCmd {
4444
.get_many::<String>("files")
4545
.expect("Must supply files")
4646
.map(|p| -> Result<PathBuf, OxenError> {
47-
let current_dir = std::env::current_dir().map_err(|e| {
48-
log::warn!("Failed to get current directory: {e}");
49-
OxenError::basic_str(format!("Failed to get current directory: {e}"))
50-
})?;
47+
let current_dir = std::env::current_dir()?;
5148
let joined_path = current_dir.join(p);
5249

5350
util::fs::canonicalize(&joined_path).or_else(|_| Ok(joined_path))

crates/cli/src/cmd/branch.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ impl RunCmd for BranchCmd {
8383
if let Some(subcommand) = args.subcommand() {
8484
match subcommand {
8585
(unlock::NAME, args) => unlock::BranchUnlockCmd.run(args).await,
86-
(cmd, _) => Err(OxenError::basic_str(format!("Unknown subcommand {cmd}"))),
86+
(cmd, _) => Err(OxenError::unknown_subcommand("branch", cmd)),
8787
}
8888
} else if args.get_flag("all") {
8989
self.list_all_branches(&repo).await
@@ -202,9 +202,7 @@ impl BranchCmd {
202202
let remote = repo
203203
.get_remote(remote_name)
204204
.ok_or(OxenError::remote_not_set(remote_name))?;
205-
let remote_repo = api::client::repositories::get_by_remote(&remote)
206-
.await?
207-
.ok_or(OxenError::remote_not_found(remote.clone()))?;
205+
let remote_repo = api::client::repositories::get_by_remote(&remote).await?;
208206

209207
let branches = api::client::branches::list(&remote_repo).await?;
210208
for branch in branches.iter() {

crates/cli/src/cmd/branch/unlock.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,7 @@ impl RunCmd for BranchUnlockCmd {
4848
let remote = repository
4949
.get_remote(remote_name)
5050
.ok_or(OxenError::remote_not_set(remote_name))?;
51-
let remote_repo = api::client::repositories::get_by_remote(&remote)
52-
.await?
53-
.ok_or(OxenError::remote_not_found(remote.clone()))?;
51+
let remote_repo = api::client::repositories::get_by_remote(&remote).await?;
5452

5553
// Unlock the branch
5654
api::client::branches::unlock(&remote_repo, branch).await?;

crates/cli/src/cmd/clone.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,12 @@ impl RunCmd for CloneCmd {
100100
.collect();
101101
let depth: Option<i32> = args
102102
.get_one::<String>("depth")
103-
.map(|s| s.parse().expect("Invalid depth, must be an integer"));
103+
.map(|s| s.parse::<i32>().map_err(OxenError::ParseIntError))
104+
.transpose()?;
104105
let is_vfs = args.get_flag("vfs");
105106
let is_remote = args.get_flag("remote");
106107

107-
let current_dir = std::env::current_dir().expect("Could not get current working directory");
108+
let current_dir = std::env::current_dir()?;
108109
let dst: PathBuf = match args.get_one::<String>("DESTINATION") {
109110
Some(dir_name) => {
110111
let path = Path::new(dir_name);

crates/cli/src/cmd/commit.rs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,8 @@ fn get_message_from_editor(maybe_config: Option<&UserConfig>) -> Result<String,
116116
# Lines starting with '#' will be ignored, and an empty message aborts the commit.\n";
117117

118118
{
119-
let mut file = std::fs::File::create(&temp_path)
120-
.map_err(|e| OxenError::basic_str(format!("Failed to create temp file: {e}")))?;
121-
file.write_all(template.as_bytes())
122-
.map_err(|e| OxenError::basic_str(format!("Failed to write to temp file: {e}")))?;
119+
let mut file = std::fs::File::create(&temp_path)?;
120+
file.write_all(template.as_bytes())?;
123121
}
124122

125123
// Spawn the editor
@@ -133,8 +131,7 @@ fn get_message_from_editor(maybe_config: Option<&UserConfig>) -> Result<String,
133131
let status = std::process::Command::new(parts[0])
134132
.args(&parts[1..])
135133
.arg(&temp_path)
136-
.status()
137-
.map_err(|e| OxenError::basic_str(format!("Failed to open editor '{editor}': {e}")))?;
134+
.status()?;
138135

139136
if !status.success() {
140137
return Err(OxenError::basic_str(format!(
@@ -143,8 +140,7 @@ fn get_message_from_editor(maybe_config: Option<&UserConfig>) -> Result<String,
143140
}
144141

145142
// Read the file and strip comments
146-
let contents = std::fs::read_to_string(&temp_path)
147-
.map_err(|e| OxenError::basic_str(format!("Failed to read temp file: {e}")))?;
143+
let contents = std::fs::read_to_string(&temp_path)?;
148144
let _ = std::fs::remove_file(&temp_path);
149145

150146
let message: String = contents

crates/cli/src/cmd/config.rs

Lines changed: 12 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -101,77 +101,38 @@ impl RunCmd for ConfigCmd {
101101
async fn run(&self, args: &clap::ArgMatches) -> Result<(), OxenError> {
102102
// Non-Repo Dependent
103103
if let Some(name) = args.get_one::<String>("name") {
104-
match self.set_user_name(name) {
105-
Ok(_) => {}
106-
Err(err) => {
107-
eprintln!("{err}")
108-
}
109-
}
104+
self.set_user_name(name)?;
110105
}
111106

112107
if let Some(email) = args.get_one::<String>("email") {
113-
match self.set_user_email(email) {
114-
Ok(_) => {}
115-
Err(err) => {
116-
eprintln!("{err}")
117-
}
118-
}
108+
self.set_user_email(email)?;
119109
}
120110

121111
if let Some(auth) = args.get_many::<String>("auth-token") {
122-
if let [host, token] = auth.collect::<Vec<_>>()[..] {
123-
match self.set_auth_token(host, token) {
124-
Ok(_) => {}
125-
Err(err) => {
126-
eprintln!("{err}")
127-
}
128-
}
129-
} else {
130-
eprintln!("invalid arguments for --auth");
131-
}
112+
let values: Vec<_> = auth.collect();
113+
// clap enforces number_of_values(2), so this is guaranteed
114+
self.set_auth_token(values[0], values[1])?;
132115
}
133116

134117
if let Some(default_host) = args.get_one::<String>("default-host") {
135-
match self.set_default_host(default_host) {
136-
Ok(_) => {}
137-
Err(err) => {
138-
eprintln!("{err}")
139-
}
140-
}
118+
self.set_default_host(default_host)?;
141119
}
142120

143121
if let Some(editor) = args.get_one::<String>("editor") {
144-
match self.set_editor(editor) {
145-
Ok(_) => {}
146-
Err(err) => {
147-
eprintln!("{err}")
148-
}
149-
}
122+
self.set_editor(editor)?;
150123
}
151124

152125
// Repo Dependent
153126
if let Some(remote) = args.get_many::<String>("set-remote") {
154127
let mut repo = LocalRepository::from_current_dir()?;
155-
if let [name, url] = remote.collect::<Vec<_>>()[..] {
156-
match self.set_remote(&mut repo, name, url) {
157-
Ok(_) => {}
158-
Err(err) => {
159-
eprintln!("{err}")
160-
}
161-
}
162-
} else {
163-
eprintln!("invalid arguments for --set-remote");
164-
}
128+
let values: Vec<_> = remote.collect();
129+
// clap enforces number_of_values(2), so this is guaranteed
130+
self.set_remote(&mut repo, values[0], values[1])?;
165131
}
166132

167133
if let Some(name) = args.get_one::<String>("delete-remote") {
168134
let mut repo = LocalRepository::from_current_dir()?;
169-
match self.delete_remote(&mut repo, name) {
170-
Ok(_) => {}
171-
Err(err) => {
172-
eprintln!("{err}")
173-
}
174-
}
135+
self.delete_remote(&mut repo, name)?;
175136
}
176137

177138
if let Some(path) = args
@@ -192,9 +153,7 @@ impl RunCmd for ConfigCmd {
192153
let storage_opts = StorageOpts::from_args(Some(backend), Some(path), bucket)?;
193154

194155
let mut repo = LocalRepository::from_current_dir()?;
195-
self.set_version_store(&mut repo, storage_opts)
196-
.await
197-
.map_err(|e| OxenError::basic_str(format!("{e}")))?;
156+
self.set_version_store(&mut repo, storage_opts).await?;
198157
}
199158

200159
Ok(())

crates/cli/src/cmd/db.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,17 @@ impl RunCmd for DbCmd {
4343
let sub_commands = self.get_subcommands();
4444
if let Some((name, sub_matches)) = args.subcommand() {
4545
let Some(cmd) = sub_commands.get(name) else {
46-
eprintln!("Unknown schema subcommand {name}");
47-
return Err(OxenError::basic_str(format!(
48-
"Unknown schema subcommand {name}"
49-
)));
46+
return Err(OxenError::unknown_subcommand("db", name));
5047
};
5148

5249
// Calling await within an await is making it complain?
5350
tokio::task::block_in_place(|| {
5451
tokio::runtime::Handle::current().block_on(cmd.run(sub_matches))
5552
})?;
5653
} else {
57-
return Err(OxenError::basic_str("No subcommand provided"));
54+
return Err(OxenError::basic_str(
55+
"No db subcommand provided. Run `oxen db --help` for usage.",
56+
));
5857
}
5958

6059
Ok(())

0 commit comments

Comments
 (0)