-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Removes the panic, throws a warning & returns an empty list, which should be expected behavior #73
Removes the panic, throws a warning & returns an empty list, which should be expected behavior #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does improve the error message and provide a possible fix, however I think the panic should be avoided.
src/main.rs
Outdated
@@ -178,7 +178,7 @@ fn main() -> crossterm::Result<()> { | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(());
}
@@ -177,8 +177,13 @@ fn main() -> crossterm::Result<()> { | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(improving the explanation) the or_else
will return another Result which has to be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
Removes the panic, throws a warning & returns an empty list, which should be expected behavior.
-> Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
447e976
to
4e680a0
Compare
fix: #55