Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

add and export path_abs crate #20

Closed
vitiral opened this issue Jan 22, 2018 · 10 comments
Closed

add and export path_abs crate #20

vitiral opened this issue Jan 22, 2018 · 10 comments

Comments

@vitiral
Copy link
Contributor

vitiral commented Jan 22, 2018

I am the creator and maintainer of the new path_abs crate and I think it solves a lot of ergonomic problems in rust (problems I see you re-solving with the fs module!).

Questions:

  • should we export path_abs types?
  • should fs module be deprecated?

Thanks!

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

@vitiral vitiral changed the title add and rexport path_abs crate add and export path_abs crate Jan 22, 2018
@killercup
Copy link
Owner

That's a very cool crate! I might use it in some of my projects :)

I only added read_file and write_to_file functions for now. What other operations that you typically have in CLI tools are covered by path_abs? And: Can it give really nice errors? :)

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

it uses std::io::Error for its errors, so probably not "that nice" haha.

The benefit of path_abs is that it makes composition easier. You can write a function that takes a file as an input (instead of a path). That file is guaranteed to exist and be absolute! Also, the file itself is easy to make (PathFile::create(path)) and read/write to (file.read_string(), file.write_str()). It basically synthesises std::fs into a couple simple types instead of being spread out over methods + types + builders.

I am questioning a bit whether this crate should rexport much outside of the initial main experience though. I'm writing my thoughts in #19

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

closing per #19

@vitiral vitiral closed this as completed Jan 22, 2018
@killercup
Copy link
Owner

Yeah, it's a really elegant design – I also like that you added a mock function for testing! This is really useful to have. I feel like this crate could also really profit from TryFrom/TryInto becoming stable!

it uses std::io::Error for its errors, so probably not "that nice" haha.

Ugh, yeah, they sadly carry a little less context information than I want to have… Why'd you chose to go with io:Error? Performance?

Oh—Here's a weird idea for errors (and totally off topic of course): Can you make an error type like struct PathFileError<T>(PathFile, PhantomData<T>) that you turn your PathFile into when an operation errors where T is one of a few given error cases (just unit struct)? That way you can impl Error for PathFileError and still get a nice error message.

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

The main reason is to mimick std::fs as much as possible, so that path_abs can be used in-place of std::fs operations. The issue is primarily one of ergonomics: for functions that return std::io::Error I would like to be able to use plain ?.

However... that is an extremely interesting thought. Before I go too far into it, your main problem with std::fs::Error is that it drops the path where the error occured right? (that's pretty much my only issue with it).

If that is the case, I could see a point of having a path_err crate which pretty much just defines (roughly) the struct you said and mimicks std::io::Error otherwise (i.e. it's kind() returns std::io::ErrorKind). This would requre calling map_err everywhere where an io::Error was returned though, which would be rather annoying...

Hmm... or we could create an alternate fs crate which has this error type as well as equivalent types, but which return the custom error type which ALWAYS contains the path. I.e. it would have a File object which could be cast into std::fs::File but which holds its path information. The annoying thing is that although you could create a "better" Write trait (i.e. WriteFile) as well as implement std::io::Write on your new File type, anything that used std::io::Write would still give the same old boring std::io::Error.

I'm not 100% sure I can see what needs to be made, but I agree that the current error messages are bad. I've had multiple occaisions where I've been pissed the file information was not included automatically (I get why it isn't for performance reasons).

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

huh, I just realized that the signature of io::Error::new is not what I thought:

pub fn new<E>(kind: ErrorKind, error: E) -> Error where
    E: Into<Box<Error + Send + Sync>>, 

It looks like it can take any type which implements error. I thought it could only take a &static str! So I CAN make io::Error pretty!

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

use std::io;
use std::fs;

fn read_dne() -> io::Result<()> {
    let path = "foobar";
    fs::File::open(path)
        .map_err(|err| io::Error::new(err.kind(), format!("(path={}) {}", path, err)))?;
    Ok(())
}

fn main() {
    if let Err(e) = read_dne() {
        println!("Got Error: {}", e);
    }
}

prints

Got Error: (path=foobar) No such file or directory (os error 2)

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

I could even do something like No such file or directory (os error 2) when reading path=foobar, which would be really clean.

@vitiral
Copy link
Contributor Author

vitiral commented Jan 22, 2018

I've been thinking of adding a PathOpen which mimicks an open File but has the path attached to it. This pretty much solidifies it for me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants