Unwrap

[uiri]

• Clean up all the unwraps, except for one around a Regex which looks rather hairy to avoid.
• Use lazy_static to more efficiently compile the regular expression.
• One pattern with converting a &Path filename to a &str involved two uses of unwrap(). I cleaned this up into a macro which is shared across modules.
• A return_on_err macro was duplicated in two modules. It has been deduplicated.
• A new Error type was created for one of the unwraps

---

This change is 

[indiv0]

Reviewed 11 of 11 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/util.rs, line 16 at r1 (raw file):

lazy_static! {
    pub static ref INBOX_RE : Regex = Regex::new("INBOX").unwrap();

If we plan to continue using the regex, I think this is fine (there's no way around the unwrap() that comes to mind), but I think we can drop this regex. We could just perform a String::contains check if we just need to match on "INBOX". If this regex will become more complicated in the future we can leave it in, but if not I'd prefer to remove it.

---

src/util.rs, line 24 at r1 (raw file):

        match $p.file_name() {
            Some(filename) =>
                match filename.to_str() {

Just a note, but stuff like this can be made more concise with methods like filename.to_str().unwrap_or_else(|| "").

---

src/util.rs, line 39 at r1 (raw file):

            let mut abs_path = absp.clone();
            abs_path.push(dir);
            abs_path.as_path().display().to_string()

I believe this line can be substituted with something like abs_path.to_str().ok_or_else(|| Error::NonUtf8Path)?.to_string().

Particularly I'm concerned about the use of display() to get the path here, because IIRC display is meant for printing. Unless this function is just used to print the path in which case :ok_hand:

---

Comments from Reviewable

[uiri]

Review status: 6 of 10 files reviewed at latest revision, 3 unresolved discussions.

---

src/util.rs, line 16 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

If we plan to continue using the regex, I think this is fine (there's no way around the `unwrap()` that comes to mind), but I think we can drop this regex. We could just perform a `String::contains` check if we just need to match on "INBOX". If this regex will become more complicated in the future we can leave it in, but if not I'd prefer to remove it.

String::replace is actually what we needed. Changed to use that and this has consequently been removed.

---

src/util.rs, line 39 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

I believe this line can be substituted with something like `abs_path.to_str().ok_or_else(|| Error::NonUtf8Path)?.to_string()`. Particularly I'm concerned about the use of `display()` to get the path here, because IIRC display is meant for printing. Unless this function is just used to print the path in which case :ok_hand:

I'm using display() here to format the Path to a UTF-8 string. That seems to be the use case for it (usually for the purposes of printing that string but whatever). I think this is preferable to erroring out on a NonUtf8Path.

---

Comments from Reviewable

[indiv0]

Reviewed 4 of 5 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/util.rs, line 39 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

I'm using `display()` here to format the `Path` to a UTF-8 string. That seems to be the use case for it (usually for the purposes of printing that string but whatever). I think this is preferable to erroring out on a NonUtf8Path.

Ah, I see. Makes sense then.

---

Comments from Reviewable

[indiv0]

So I think I messed up by responding to your comment instead of acknowledging it, so reviewable thinks this is still unresolved. I'm just gonna and merge this.