Improve error handling #299

Kibouo · 2020-03-21T18:10:54Z

PR for #291

Currently adding error messages based on context. However, I'm not familiar with the code base so thorough review is highly recommended to improve error messages
... especially for the fzf and core modules.

TODO:

Every call to bash now has an error message similar to "Failed to execute bash". This should probably be related to the actual command ran, i.e. params given to bash.
A lot of anyhow::[with_]context() is used to add, well, context. It might be better to create custom error variants for repetitive errors.
navi fn welcome is broken.
Add "file a bug report" message to top-level error.
Test for potential breakages. Logic changes have been avoided as much as possible but stuff happens.
navi repo browse and navi repo add https://github.com/denisidoro/navi crash. Root cause is fzf not returning data due to window layout error.
bump version

welcome · 2020-03-21T18:10:56Z

Thanks for opening this pull request!

Kibouo · 2020-03-21T18:18:21Z

My approach currently is to:

eliminate all unwrap(), except in tests
expect() when calling an internal function with highly controlled input, since these "ugly" panic messages ~~will~~ should only ever be seen by devs.
return as much Result as possible, as the added context to each layer of errors will be nicely formatted and hopefully useful for the end-user.

denisidoro

👏 👏 👏

I've learned a lot reading this. Thank you for your contribution!

…item + add test for this

…en input is empty.

…pose is to make place for a replacement.

denisidoro · 2020-03-22T23:19:42Z

The navi repo bug is fixed in master 👍

Kibouo · 2020-03-22T23:23:28Z

Kindly re-review changes. Please check whether the actual messages are clear.

Also added a wrapping error telling the end-user to file an issue. Ex:

This required the error type FileAnIssue to be made public in the library. It had to be used in the bin. handle_config is called multiple times so it would look weird to potentially have multiple of these messages nested. Is this change ok?

A few things remain to be done as can be seen in the TODO of the first post.

…dling

Kibouo · 2020-03-22T23:28:46Z

Would this be a bump in version to 2.4.0?

denisidoro

Amazing work!

I've never thought that error messages could serve as "documentation" for the source code.

My future work will surely be influenced by this PR.

Thanks again!

denisidoro · 2020-03-23T00:40:02Z

src/filesystem.rs

+    io::BufReader::new(file)
+        .lines()
+        .map(|line| line.map_err(Error::from))
+        .collect::<Result<Vec<String>, Error>>()


I'm not well versed in Rust, but won't this collect make the collection eager? If the file being parsed has 10,000 lines, won't this make the function read all these lines before we can process the first one?

In the Err case it short circuits. But Ok case it indeed allocates unnecessarily. Fixed!

denisidoro · 2020-03-23T01:02:19Z

src/flows/repo.rs

+    // The dir might not exist which would throw an error. But here we don't care about that
+    // since our purpose is to make space for a new directory. Potential other errors
+    // (e.g due to lack of permission) will cause an error during dir creation.
+    let _ = filesystem::remove_dir(&repo_path_str);


Why using let _ =?

Rust requires you to use the return value of remove_dir. But as stated in the comment above it, we don't care about the potential Err value. And the Ok type is () anyway.

denisidoro · 2020-03-23T01:02:59Z

src/flows/repo.rs


-    filesystem::remove_dir(&repo_path_str);
-    filesystem::create_dir(&repo_path_str);
+    // The dir might not exist which would throw an error. But here we don't care about that


Do you suggest another approach? I agree this is very error-prone

Checking whether the dir exists before trying to remove it is a bit more robust, but not worth it imho. The check + remove is not atomic so remove_dir would still be able to return the same errors. Just less frequently.

denisidoro · 2020-03-23T01:14:39Z

src/handler.rs

-            Search { query } => flows::search::main(query.clone(), config),
+
+            Query { query } => {
+                let error_string = format!("Failed to filter cheatsheets for {}", &query);


Wouldn't with_context prevent us from generating this str 100% of the times?

query is part of config. But config is given away to flows::query::main on :17, which makes it impossible to use query after that. So we need to make a copy of query before that happens.

let query_clone = query.clone() and then passing that to with_context would be a possibility. Not sure how smart the compiler is. I'll change it to be sure 🤔

denisidoro · 2020-03-23T01:19:59Z

src/parser.rs

+            } else if let [flag] = flag_and_value {
+                Err(anyhow!("No value provided for the flag `{}`", flag))
+            } else {
+                unreachable!() // Chunking by 2 allows only for tuples of 1 or 2 items...


Why not use an else in the line 72 instead?

The chunks(2) call on :46 returns slices (&[..]) of the collection. The problem is that, despite telling chunks that the slices should be 2 long, it doesn't know this at compile time. So Rust wants us to handle every possible case of slice length exhaustively.

Concrete this means that Rust would want the following let binding to be exhaustive.

} else { let [flag] = flag_and_value; Err(anyhow!("No value provided for the flag `{}`", flag)) }

You could do the following but obviously it wouldn't be as helpful an error:

} else { Err(anyhow!("No value provided for a flag")) }

denisidoro · 2020-03-23T01:22:25Z

src/structures/error/file_issue.rs

+
+#[derive(Error, Debug)]
+#[error(
+    "\rHey listen! Navi encountered a problem.


welcome · 2020-03-23T19:35:28Z

Congrats on merging your first pull request!

Kibouo added 13 commits March 20, 2020 22:51

Add anyhow dependency.

70ec05b

Add context for errors when browsing featured cheatsheets.

1d33211

Add context to possible errors for the repo subcommand.

f118227

Add error info to preview::extract_elements

0a84a0a

Add error context to ad-hoc functions.

54fec87

Add error info for welcome handling.

a209dd9

Fix error message when spawning bash in func.

77d0de8

Add error info when getting terminal dimensions.

d371300

Add error info for invalid regex.

0785f9c

Make all regexes static.

807c3a3

Add error info to core flow.

ff26473

Add error info to filesystem operations.

a4e2311

Add error info to fzf.

1c22e11

Kibouo requested a review from denisidoro as a code owner March 21, 2020 18:10

Make error info for cheatsheet paths acquisition more verbose.

c1c5910

denisidoro approved these changes Mar 21, 2020

View reviewed changes

Kibouo added 12 commits March 22, 2020 01:51

Add error context to parser.

07509dd

Make shell handler return an anyhow::Error.

a4c30d7

Fix parsing of paths param with trailing colon to not yield an empty …

3c5d5ab

…item + add test for this

Added backticks around variable data in error info. Less ambiguous wh…

800a7e6

…en input is empty.

Fix error message for absense of cheatsheet dir

5c44bba

Add thiserror dependency

38b85fa

Add InvalidPath error

4bbccc2

Restructure error module

11575a8

Add UnreadableDir error.

5d2c2ac

Add BashSpawnError

4bfc2e5

Fix welcome command

a625b0a

Add wrapper error telling the user to file an issue

4bc9b00

Kibouo added 2 commits March 22, 2020 23:38

Fix "file an issue" error

09dfc32

Fix remove dir panicing when the dir does not exist, but when the pur…

ec0bbf4

…pose is to make place for a replacement.

Kibouo requested a review from denisidoro March 22, 2020 23:17

Kibouo added 2 commits March 23, 2020 00:24

Add context to file copy when pulling cheatsheets from a repo.

ed8e3e4

Merge remote-tracking branch 'upstream/master' into improve-error-han…

80621db

…dling

Bump version to 2.4.0

1c7c293

Kibouo changed the title ~~[WIP] Improve error handling~~ Improve error handling Mar 23, 2020

denisidoro approved these changes Mar 23, 2020

View reviewed changes

Kibouo added 3 commits March 23, 2020 16:38

Remove unneeded mut

f214a56

Potentialy fix error string creation to only be done when needed.

8c4100a

Fix unnecessary allocations when reading file.

53c69aa

denisidoro merged commit 7734723 into denisidoro:master Mar 23, 2020

Kibouo mentioned this pull request Mar 23, 2020

Improve error reporting #291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling #299

Improve error handling #299

Kibouo commented Mar 21, 2020 •

edited

Loading

welcome bot commented Mar 21, 2020

Kibouo commented Mar 21, 2020

denisidoro left a comment

denisidoro commented Mar 22, 2020

Kibouo commented Mar 22, 2020

Kibouo commented Mar 22, 2020

denisidoro left a comment •

edited

Loading

denisidoro Mar 23, 2020

Kibouo Mar 23, 2020 •

edited

Loading

denisidoro Mar 23, 2020

Kibouo Mar 23, 2020

denisidoro Mar 23, 2020

Kibouo Mar 23, 2020

denisidoro Mar 23, 2020

Kibouo Mar 23, 2020

denisidoro Mar 23, 2020

Kibouo Mar 23, 2020 •

edited

Loading

denisidoro Mar 23, 2020

welcome bot commented Mar 23, 2020

Improve error handling #299

Improve error handling #299

Conversation

Kibouo commented Mar 21, 2020 • edited Loading

welcome bot commented Mar 21, 2020

Kibouo commented Mar 21, 2020

denisidoro left a comment

Choose a reason for hiding this comment

denisidoro commented Mar 22, 2020

Kibouo commented Mar 22, 2020

Kibouo commented Mar 22, 2020

denisidoro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kibouo Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kibouo Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welcome bot commented Mar 23, 2020

Kibouo commented Mar 21, 2020 •

edited

Loading

denisidoro left a comment •

edited

Loading

Kibouo Mar 23, 2020 •

edited

Loading

Kibouo Mar 23, 2020 •

edited

Loading