Skip to content
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

src: fix clippy warnings #234

Merged
merged 2 commits into from
Aug 26, 2020
Merged

src: fix clippy warnings #234

merged 2 commits into from
Aug 26, 2020

Conversation

Fishrock123
Copy link
Member

This should unblock CI on main.

@@ -80,6 +80,7 @@ mod test {
use std::time::Duration;

#[test]
#[allow(clippy::redundant_clone)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why allow instead of resolve? is this more complex than removing the last .clone()?

Copy link
Member Author

@Fishrock123 Fishrock123 Aug 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it made it more consistent to read...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was probably catched by your editor but not by the CI which does not clippy the tests.
Maybe you could consider using clippy --test instead of clippy in the the CI to catch those issues earlier ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? I run the same command as CI runs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange indeed, On my side I only get the redundant closure error when running cargo clippy --tests. Running rust stable 1.45.2 and nightly 1.47.0-nightly (f44c6e4e2 2020-08-24).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to say I am running Windows 10, I will try on Linux tonight

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating this further I see that cargo clippy --all-targets also activate the --tests flag. This is maybe the command you are running ? When running only cargo clippy -- -D warnings (used by CI) I do not get the errors from #[cfg(test)] guarded code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nightly-x86_64-pc-windows-msvc (overridden by +toolchain on the command line)
rustc 1.47.0-nightly (5180f3da5 2020-08-23)
cargo +nightly clippy --tests --examples -- -D warnings

Huh, turns out that's what Tide runs but not this. Adding a commit...

@Fishrock123
Copy link
Member Author

Fixed something that was in an even newer version of clippy.

This should unblock CI on main.
@Fishrock123 Fishrock123 changed the title ServerTiming: fix clippy warnings src: fix clippy warnings Aug 25, 2020
Same as Tide.

See comments in http-rs#234
@Fishrock123
Copy link
Member Author

Ok I am merging this to unblock CI

@Fishrock123 Fishrock123 merged commit 89bcc45 into http-rs:main Aug 26, 2020
@Fishrock123 Fishrock123 deleted the fix-clippy branch August 26, 2020 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants