clippy: Apply clippy suggestions#3216
Conversation
`cargo clippy` says it is better to use `Box::default()` directly. https://rust-lang.github.io/rust-clippy/master/index.html#box_default `CONTRIBUTING.md` suggests one runs this, before publishing a change: ```bash cargo clippy --all --all-targets --all-features -- -D warnings ``` So it would be great if `master` would not generate any warnings.
`cargo clippy` says it is better to use types that have less complex structure. Introducing a type alias does provide an additional intermediate name, that could improve readability a bit. https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity `CONTRIBUTING.md` suggests one runs this, before publishing a change: ```bash cargo clippy --all --all-targets --all-features -- -D warnings ``` This is the last warning, at the moment. Applying this fix makes `cargo clippy` output warning free.
illia-bobyr
force-pushed
the
clippy-warnings
branch
from
92fe3d7 to
82b0e43
Compare
0xPoe
left a comment
0xPoe
left a comment
There was a problem hiding this comment.
Thanks for your PR.
We have some discussion about the Box::default() change before. Maybe we can revert it.
| // building with i686 toolchain, but on an x86_64 host, so run the | ||
| // actual detection logic and trust it. | ||
| let tp = Box::new(currentprocess::TestProcess::default()); | ||
| let tp = Box::<currentprocess::TestProcess>::default(); |
There was a problem hiding this comment.
Definitely can, but it is what the stable clippy is suggesting today.
During that discussion, it was still a nightly only check.
Maybe, instead of just removing the patch, I should change it to
#[allow(clippy::box_default)]For this particular location. With a link to that discussion?
Otherwise, it may come again, as clippy will keep complaining :)
There was a problem hiding this comment.
I totally agree with the consistency argument.
But, if this is what clippy is suggesting, I wonder if most new code will be written like this now.
There was a problem hiding this comment.
I would like to see a blanket allow on this for rustup.
| path: &Path, | ||
| edit: Option<&dyn Fn(&str, &mut MockChannel)>, | ||
| ) -> MockDistServer { | ||
| pub fn create_mock_dist_server(path: &Path, edit: Option<&EditChannel>) -> MockDistServer { |
There was a problem hiding this comment.
So EditChannel is just defining the function signature that these two functions use as a closure.
We're separating out two things that are tightly coupled. I don't think that increases readability: it increases cognitive load.
Please allow() it at the module level. (I have a PR in flight that refactors this test code towards immutability, so the whole edit concept is going to fade anyhow).
|
Already done at https://github.com/rust-lang/rustup/pull/3276/files. So I am going to close this PR. Thanks for your contribution! |
|
@gautamprikshit1 I would like to suggest asking the original author before picking up the work from their PRs next time. This way people don't race on the same thing and the original author may feel better. ❤️ Thank you all. |
3 participants
CONTRIBUTING.mdsuggests one runs this, before publishing a change:These changes make
cargo clippyoutput onmasterwarning free.