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
core: test Modules::deps and handle error cases better #2141
Conversation
core/modules.rs
Outdated
@@ -355,12 +364,25 @@ impl Deps { | |||
|
|||
Self::helper(seen, new_prefix, new_is_last, modules, dep_name) | |||
}).collect(); | |||
Deps { | |||
|
|||
// If any of the children are missing, return None. |
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.
Some notes on this section (up to line 378):
- I get the clippy warning:
this block may be rewritten with the
?operator
. Not sure what it has in mind exactly though. - The whole thing can be written shorter and more efficiently by relying on the fact that the
collect()
method can turn an iterator overOption<T>
into anOption<Vec<T>
(this also works forResult
and for different types of collections). Replace it with this:let deps = maybe_deps.into_iter().collect::<Option<_>>()?;
@@ -321,7 +325,7 @@ pub struct Deps { | |||
} | |||
|
|||
impl Deps { | |||
pub fn new(modules: &Modules, module_name: &str) -> Deps { | |||
fn new(modules: &Modules, module_name: &str) -> Option<Deps> { |
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.
If this function returns an Option then I think the field Deps::deps shouldn't be an Option any more.
core/modules.rs
Outdated
} else { | ||
let maybe_children = modules.get_children2(name); |
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 block may be rewritten with the `?` operator
note: #[warn(clippy::question_mark)] on by default
help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark
help: replace_it_with: `maybe_children?;`
core/modules.rs
Outdated
let child_count = children.iter().count(); | ||
let deps = children | ||
let child_count = children.len(); | ||
let mut maybe_deps: Vec<Option<Deps>> = children |
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.
variable does not need to be mutable
note: #[warn(unused_mut)] on by default
help: remove this `mut`: ``
Suggestions for improvement: 6b240844 |
@@ -354,13 +358,16 @@ impl Deps { | |||
new_prefix.push(' '); |
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.
The construction of this string would be more readable using format!()
.
It should also preferrably move out of the closure.
I'm getting link errors locally on this branch - apparently not present in travis
|
The above link errors were caused by building with 1.33.0 - it works with 1.34.0 ... |
cc @fewf