Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create src/errors.rs with generic errors for the entire crate #215

Closed
dlaehnemann opened this issue Jun 14, 2020 · 2 comments
Closed

create src/errors.rs with generic errors for the entire crate #215

dlaehnemann opened this issue Jun 14, 2020 · 2 comments

Comments

@dlaehnemann
Copy link
Member

In PR #165 (now superseded by PR #214 ), the review led to the idea of defining generic errors like FileNotFound and NonUnicodePath on the highest level of the crate, i.e. in a new file src/errors.rs. This avoids redundantly defining such errors for all submodules. Currently, the two example errors are defined both for bam, tbx and faidx:

#[snafu(display("file not found: {}", path.display()))]
FileNotFound { path: PathBuf },
#[snafu(display("invalid (non-unique) characters in path"))]
NonUnicodePath,

#[snafu(display("file not found: {}", path.display()))]
FileNotFound { path: PathBuf },
#[snafu(display("invalid (non-unique) characters in path"))]
NonUnicodePath,

#[snafu(display("file not found: {}", path.display()))]
FileNotFound { path: PathBuf },
#[snafu(display("invalid (non-unicode) characters in path"))]
NonUnicodePath,

And NonUnicodePath is also defined for bcf, but FileNotFound is missing there. So such high-level definition could also help clean up the errors a bit:

#[snafu(display("invalid (non-unique) characters in path"))]
NonUnicodePath,

@dlaehnemann
Copy link
Member Author

While this change happens, we could also think about moving the path_as_bytes function up to src/utils.rs:

fn path_as_bytes<'a, P: 'a + AsRef<Path>>(path: P, must_exist: bool) -> Result<Vec<u8>> {
if path.as_ref().exists() || !must_exist {
Ok(path
.as_ref()
.to_str()
.ok_or(Error::NonUnicodePath)?
.as_bytes()
.to_owned())
} else {
Err(Error::FileNotFound {
path: path.as_ref().to_owned(),
})
}
}

@landesfeind
Copy link
Contributor

landesfeind commented Aug 6, 2020

As discussed, I started working on this. First draft is available in landesfeind/rust-htslib:215_generic_errors_for_entire_crate

While there are clearly some generic errors, others are also quite specific to the modules. I am wondering if there is a preference in using specialized errors with a prefix (i.e., what I did in a first merge to solve name-conflicts) like BcfSeek or if we should try to harmonize/generalize them (e.g., GenomicSeek { target: String, position: u64 } versus FileSeek)

Any thoughts?

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

No branches or pull requests

3 participants