Datagen API polish #3951
Datagen API polish #3951
Conversation
|
(happy to review if needed but at this point there's a lot of stuff going on in datagen and I've only kept up with the high level things) |
| #[arg(help = "Load a JSON config. All options other than --verbose are ignored.")] | ||
| config: Option<PathBuf>, |
There was a problem hiding this comment.
Observation: you made this a positional argument
provider/datagen/src/lib.rs
Outdated
| /// | [`Preresolved`] | No | Small | | ||
| /// | [`Runtime`] | Required, Automatic | Smallest | | ||
| /// | [`RuntimeManual`] | Required, Manual | Smallest | | ||
| /// | [`Preresolved`] | Not required | Small | |
There was a problem hiding this comment.
The problem with runtime fallback on preresolved is that we don't guarantee that things like en-001 are present. You really shouldn't be using preresolved if you plan on runtime fallback.
There was a problem hiding this comment.
do you have a suggestion? I'd rather say required for the first two
There was a problem hiding this comment.
Now about "N/A" or "Not applicable"
There was a problem hiding this comment.
Not supported? If it's actually a problem we should discourage people from doing it
| /// fail with the appropriate error: | ||
| /// * [`is_missing_cldr_error`](Self::is_missing_cldr_error) | ||
| /// * [`is_missing_icuexport_error`](Self::is_missing_icuexport_error) | ||
| /// * [`is_missing_segmenter_lstm_error`](Self::is_missing_segmenter_lstm_error) |
There was a problem hiding this comment.
Why did you move these onto DatagenProvider? I think they were good as free functions in the error module
There was a problem hiding this comment.
Free functions are messy. These errors are a property of the DatagenProvider, not of any other part of datagen.
#3564