Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
dwijnand
left a comment
dwijnand
left a comment
There was a problem hiding this comment.
Nice! A few (simple) comments and questions from me.
tests/testsuite/profile_overrides.rs
Outdated
There was a problem hiding this comment.
There's a preference in Cargo to use the former over the latter.
There was a problem hiding this comment.
Ok, I added a allow(explicit_iter_loop) to testsuites main.rs.
tests/testsuite/support/mod.rs
Outdated
There was a problem hiding this comment.
Would the use-cases stay simple if this took a p: P where P: AsRef<Path>?
tests/testsuite/config.rs
Outdated
There was a problem hiding this comment.
Is this formatting common? Looks strange to me.
There was a problem hiding this comment.
Ah oops, fallout of lazy search and replace :/
tests/testsuite/config.rs
Outdated
There was a problem hiding this comment.
Similarly, would using AsRef<CargoError> avoid changing all the existing use sites?
There was a problem hiding this comment.
Hmm I could not get these and the path2url to work with AsRef, but I can revert the change altogether if you want.
There was a problem hiding this comment.
Here's the change that worked for me on path2url:
-pub fn path2url(p: PathBuf) -> Url {
- Url::from_file_path(&*p).ok().unwrap()
+pub fn path2url<P: AsRef<Path>>(p: P) -> Url {
+ Url::from_file_path(p).ok().unwrap()
}There was a problem hiding this comment.
For assert_error:
use cargo::CargoError;
use support::{execs, lines_match, paths, project};
use support::hamcrest::assert_that;
+use std::borrow::Borrow;
use std::collections;
use std::fs;-fn assert_error(error: CargoError, msgs: &str) {
+fn assert_error<E: Borrow<CargoError>>(error: E, msgs: &str) {
let causes = error
+ .borrow()
.iter_chain()
.map(|e| e.to_string())
.collect::<Vec<_>>()
tests/testsuite/build.rs
Outdated
There was a problem hiding this comment.
these to_string()'s aren't necessary as with_stderr takes S: ToString, and thus can take a string slice.
dwijnand
left a comment
dwijnand
left a comment
There was a problem hiding this comment.
LGTM. Thanks @matthiaskrgr!
|
Thanks for the reviews! |
src/cargo/ops/fix.rs
Outdated
There was a problem hiding this comment.
Could this just be if edition == "2018" { ... }?
There was a problem hiding this comment.
Indeed, updated accordingly.
…s --all-features -- --cap-lints warn ) Special thanks to dwijnand for helping me with this! :)
|
CI is flakey again.... |
|
@bors: r+ |
|
📌 Commit 8798bf0 has been approved by |
fix a bunch of clippy warnings (invocation: cargo clippy --all-targets --all-features -- --cap-lints warn )
|
☀️ Test successful - status-appveyor, status-travis |
Update cargo - Update transitioning url (rust-lang/cargo#5889) - Resolve some clippy lint warnings (rust-lang/cargo#5884) - Don't kill child processes on normal exit on Windows (rust-lang/cargo#5887) - fix a bunch of clippy warnings (rust-lang/cargo#5876) - Add support for rustc's --error-format short (rust-lang/cargo#5879) - Support JSON with rustdoc. (rust-lang/cargo#5878) - Fix rustfmt instructions in CONTRIBUTING.md (rust-lang/cargo#5880) - Allow `cargo run` in workspaces. (rust-lang/cargo#5877) - Change target filters in workspaces. (rust-lang/cargo#5873) - Improve verbose console and log for finding git repo in package check (rust-lang/cargo#5858) - Meta rename (rust-lang/cargo#5871) - fetch: skip target tests when cross_compile is disabled (rust-lang/cargo#5870) - Fully capture rustc and rustdoc output when -Zcompile-progress is passed (rust-lang/cargo#5862) - Fix test --example docs. (rust-lang/cargo#5867) - Add a feature to build a vendored OpenSSL (rust-lang/cargo#5865)
7 participants
(invocation: cargo clippy --all-targets --all-features -- --cap-lints warn )