Adding download and locale filtering options #299
Conversation
This comment has been minimized.
This comment has been minimized.
| /// The fields should be Ok if present. They default to Err when not present. | ||
| pub trait CldrPaths { | ||
| fn cldr_core(&self) -> Result<PathBuf, Error>; | ||
| fn cldr_dates(&self) -> Result<PathBuf, Error>; |
There was a problem hiding this comment.
do you need to return PathBuf? Usually in such cases we use Path (just like we'd return &str not String).
There was a problem hiding this comment.
I think you either have an owned PathBuf or a borrowed &Path. If I return &Path, then someone has to own it.
I figured it was fine to return PathBuf because the only call site immediately takes it and starts appending more pieces to it, so if we returned &Path, we'd need to call .to_path_buf() anyway.
| /// https://github.com/unicode-cldr/cldr-core/tags | ||
| pub fn from_github_tag(github_tag: &str) -> Self { | ||
| Self { | ||
| cache_dir: dirs::cache_dir().unwrap().join("icu4x").join("cldr"), |
| #[non_exhaustive] | ||
| #[derive(Debug)] | ||
| pub enum Error { | ||
| JsonError(serde_json::error::Error), | ||
| IoError(std::io::Error, std::path::PathBuf), | ||
| MissingSource(MissingSourceError), | ||
| Download(Box<dyn error::Error>), |
There was a problem hiding this comment.
is there a reason you don't want to use DownloadError here?
There was a problem hiding this comment.
It was either use a box or make this Download(DownloadError) and protect that enum variant with #[cfg(feature = "download")] everywhere it is used in this file. I had a slight preference for a box in order to reduce the number of places where I needed #[cfg(feature = "download")]. Do you have a preference?
| @@ -1,12 +1,16 @@ | |||
| use std::error; | |||
| use std::fmt; | |||
|
|
|||
| #[cfg(feature = "download")] | |||
| use crate::download::error::Error as DownloadError; | |||
|
|
|||
There was a problem hiding this comment.
since download is optional, would it be easier to maintain to have the From in download::error instead of here?
There was a problem hiding this comment.
Perhaps, but I still need to put the Display and std::error::Error implementations in this file, so it seems odd that From would be in a separate place. It's easy to put it in this file, and I protect it with #[cfg(feature = "download")].
Key changes in this PR:
I added the download feature because:
The locale filtering feature is bare-bones for now. We can upgrade it once we figure out how we are resolving locale negotiation in data loading (#173).