-
Notifications
You must be signed in to change notification settings - Fork 888
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
Implemented support for workspaces #1413
Conversation
I believe I have fixed the errors the tests gave me. |
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.
Thanks for the PR! I think this is good. I have a few comments inline where the code could be polished. I'd also like to get someone more familiar with Cargo to review this too
src/bin/cargo-fmt.rs
Outdated
} | ||
} | ||
|
||
fn is_none(&self) -> bool { |
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 you derive Eq
and ParitalEq
then you could use ==
rather than needing the is_all
and _none
methods
src/bin/cargo-fmt.rs
Outdated
|
||
pub fn get_some<'a>(&'a self) -> Option<&'a [String]> { | ||
use std::borrow::Borrow; | ||
match self { |
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.
I would prefer to use if let
rather than match
here
src/bin/cargo-fmt.rs
Outdated
pub fn get_some<'a>(&'a self) -> Option<&'a [String]> { | ||
use std::borrow::Borrow; | ||
match self { | ||
&WorkspaceHitlist::Some(ref hitlist) => Some(hitlist.borrow()), |
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.
You should be able to use &hitlist
(or possibly &*hitlist
if that doesn't work) rather than borrow
.
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.
Yes not sure why I forgot about the deref on vecs
src/bin/cargo-fmt.rs
Outdated
@@ -63,7 +70,13 @@ fn execute() -> i32 { | |||
return success; | |||
} | |||
|
|||
match format_crate(verbosity) { | |||
let workspace_hitlist = match (matches.opt_present("all"), matches.opt_present("p")) { |
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.
I would factor this out into WorkspaceHitlist::from_matches
src/bin/cargo-fmt.rs
Outdated
.unwrap() | ||
.as_string() | ||
.unwrap(); | ||
if hitlist.take(&member_name.to_string()).is_some() { |
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.
You can remove the if
block here and just return hitlist.take(&member_name.to_string()).is_some()
I've addressed your comments @nrc and also changed the error message for |
@Emilgardis thanks for the changes. Could you rebase please? Then I will merge. |
Test errors are unrelated to my changes as far as I can see. |
Is this possibly a regression in nightly? |
Yeah, the doctest behaviour in nightly changed. I fixed this for rustfmt in a commit last night |
see #1244
cc @nrc