Removes the panic, throws a warning & returns an empty list, which should be expected behavior#73
Conversation
Sntx626
left a comment
There was a problem hiding this comment.
Does improve the error message and provide a possible fix, however I think the panic should be avoided.
src/main.rs
Outdated
| if opt.list_languages { | ||
| opt.languages() | ||
| .expect("Couldn't get installed languages under config directory. Make sure the config directory exists.") | ||
| .expect("Couldn't get installed languages under the config directory. Make sure the config and language directory exist.") |
There was a problem hiding this comment.
This improves the error message.
However I agree with @omarkohl, the message should be printed as a warning and return an empty list with .or_else(vec![]).
There was a problem hiding this comment.
Since there's no logging this could look like the following:
if opt.list_languages {
opt.languages()
.or_else({
println!("Warning: Couldn't get installed languages under config directory. Make sure the config directory exists.");
vec![]
})
.iter()
.for_each(|name| println!("{}", name.to_str().expect("Ill-formatted language name.")));
return Ok(());
}| if opt.list_languages { | ||
| opt.languages() | ||
| .expect("Couldn't get installed languages under config directory. Make sure the config directory exists.") | ||
| match opt.languages() { |
There was a problem hiding this comment.
Since or_else methods on results don't return a default value, an if or a match is necessary
There was a problem hiding this comment.
(improving the explanation) the or_else will return another Result which has to be handled.
Sntx626
left a comment
There was a problem hiding this comment.
This looks good!
Removes the panic, throws a warning & returns an empty list, which should be expected behavior.
-> Approved
447e976 to
4e680a0
Compare
fix: #55