-
Notifications
You must be signed in to change notification settings - Fork 104
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
Cleanup #275
Cleanup #275
Conversation
impl/src/bin/cargo-raze.rs
Outdated
// Clean out the "remote" directory and guarantee that it exists | ||
if remote_dir.exists() { | ||
for entry in glob::glob(&format!("{}/BUILD*.bazel", &remote_dir.display().to_string()))? { | ||
for entry in glob::glob(&format!( |
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.
Nit: I am not the biggest fan of everything_all_in_one_place(at(all(times)), so_its_hard_to_read)
Maybe if its a cleanup:
let build_glob = format!("{}/BUILD*.bazel", remote_dir.display());
for entry in glob::glob(&build_glob)? {
I also think the display().to_string()
is a bit needless, either we want display semantics (aka just use the std::path::Display
struct directly) or to_string_lossy()
is a better fit
?
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.
Updated. I might have misread what you wrote, I stuck with std::path::Display
but are you saying to_string_lossy
is better?
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.
No I was more suggesting options
@GregBowyer this is ready for another review |
👍 LGTM |
@acmcarther this PR is ready for review |
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.
Looks fine to me, but can you please update the PR to be more specific about both what is happening, and why?
@dfreese done |
This is work pulled out of #276
Changes:
settings::testing
tosettings::tests
to be consistent with other modulescargo fmt