Adding some path functions

[TonioGela]

I've tried to add the functions I talked about in #871, mirroring the standard library functions from Path.
I've added a bunch of tests, but I haven't updated the documentation.
I'm looking forward to getting this pull request reviewed because it will be my first rust contribution if it gets merged.
For the very same reason, it could be non-optimal or straightforward.
Closes #871.

[casey]

Awesome, thanks for tackling this! I probably won't be able to take a look at this today, but I should have time tomorrow. In the meantime, it looks like there are some errors from clippy on CI.

[TonioGela]

Awesome, thanks for tackling this! I probably won't be able to take a look at this today, but I should have time tomorrow. In the meantime, it looks like there are some errors from clippy on CI.

Managed it, thanks!

[casey]

Just took a quick look, and overall I think it looks good. You're running into a lot of the things that make Rust painful, for example, the fact that paths might not be valid Unicode.

I think one thing that might make this a whole lot nicer is if you use camino's Utf8Path type. That way, you can do Utf8Path::new(argument), which will always succeed, and thereafter you won't have to deal with the error case of non-unicode paths.

The other thing that would be nice would be to de-deuplicate these format calls:

    .ok_or_else(|| {
      format!(
        "Cannot get file stem from {}",
        if file_path.is_empty() {
          "an empty string"
        } else {
          file_path
        }
      )

I'm not entirely sure what the best way to do this would be, but you could do something like:

fn extraction_error_message(part: &string, path: &Utf8Path) -> String {
  format!("Cannot extract {} from `{}`, part, path)
}

If the path is set in backticks, then I think:

Cannot extract X from ``

Is a good error message.

[casey]

Actually, you could skip the function and just put format!("Cannot extract file stem from {}, path). Without the if … { … } else { … } I don't think it's complicated enough to deserve a function.

[casey]

Also, thanks for adding tests! That's something that a lot of people forget to do.

[TonioGela]

Also, thanks for adding tests! That's something that a lot of people forget to do.

My pleasure. I reimplemented the functions using camino and simplifying the error handling. I'm totally not sure about dependency management. Am I supposed to commit the Cargo.lock file too?

[TonioGela]

And btw compliments for the functions "interface", adding functionality (even coming from a totally different language like Scala) was pretty trivial since it is really well designed ;)

[casey]

Looking better! A few more comments.

As for the lock file, yes, that should be committed. The general rule is that for binary crates, such as just, the lockfile should be committed so that builds are reproducible. For library crates, the lockfile shouldn't be committed, since it's up to the binary crate that the library is eventually included in to determine dependency versions to use.

[casey]

And btw compliments for the functions "interface", adding functionality (even coming from a totally different language like Scala) was pretty trivial since it is really well designed ;)

Thank you! It's actually pretty easy to add functions that introduce new functionality, so I kind of wonder why I haven't added more over the years.

[casey]

Not sure if you saw, but I added some comments in-line, for example.

[TonioGela]

Not sure if you saw, but I added some comments in-line, for example.

Yes, I read the previous comment from the phone, and it hid the in-line comments. I think I addressed them all now.

[casey]

Looks great! How about using consistent language in all the error message, so:

Cannot extract X from `{}`

Also, instead of:

Cannot extract file_stem from `{}`

How about:

Cannot extract file stem from `{}`

[TonioGela]

Typo, my bad. Is this okay?

[casey]

I just added some suggestions, I think you can just click a button to apply them to the branch. I realized that all the error messages are in the past tense, so I switched to Could not, and also used extract as the verb for all the messages

[TonioGela]

I just added some suggestions, I think you can just click a button to apply them to the branch.

I'm not sure to see where you wrote them, I've closed the discussion for all the previous comments but the one I linked (since I'm not sure about how to add the import in common.rs). All the other problems were addressed.

I realized that all the error messages are in the past tense, so I switched to Could not, and also used extract as the verb for all the messages

I'll apply these changes asap.

[casey]

Ahhh, damn, sorry, I forgot to hit the "submit review" button, so you couldn't see any of them.

[TonioGela]

Just applied everything and verified locally that both tests and clippy are good

[casey]

Great, looks good, although I think there's a formatting issue.

[casey]

Merged!