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

tower: fix annoying clippy lints #639

Merged
merged 2 commits into from
Feb 11, 2022
Merged

tower: fix annoying clippy lints #639

merged 2 commits into from
Feb 11, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Feb 11, 2022

This fixes a bunch of minor clippy lints. None of them were particularly
major, but I was getting tired of the warnings showing up in vscode.

The one lint that had to be ignored rather than fixed is the
clippy::bool_assert_comparison lint, which triggers on the
tower_test::assert_request_eq! macro. The lint triggers when writing
code like assert_eq!(whatever, true) rather than simply
assert!(whatever). In this case, this occurs because the macro makes
an assertion about a request value, and in some tests, the request
type is bool. We can't change this to use assert!, because in most
cases, when the request is not bool, we actually do need assert_eq!,
so I ignored that warning.

This fixes a bunch of minor clippy lints. None of them were particularly
major, but I was getting tired of the warnings showing up in vscode.

The one lint that had to be ignored rather than fixed is the
`clippy::bool_assert_comparison` lint, which triggers on the
`tower_test::assert_request_eq!` macro. The lint triggers when writing
code like `assert_eq!(whatever, true)` rather than simply
`assert!(whatever)`. In this case, this occurs because the macro makes
an assertion about a request value, and in _some_ tests, the request
type is `bool`. We can't change this to use `assert!`, because in most
cases, when the request is not `bool`, we actually do need `assert_eq!`,
so I ignored that warning.
@hawkw
Copy link
Member Author

hawkw commented Feb 11, 2022

The build failure appears to be due to a compiler regression (rust-lang/rust#93821). I believe there's a PR fixing it (rust-lang/rust#93893), but it's not in yesterday's nightly.

Because our CI is currently configured with fail-fast enabled for the cross-version test jobs, we don't get to see the results of the builds on versions other than nightly, so it's not obvious that this is a nightly-specific issue. I opened #640 to change the CI configuration to not do that.

I think we may also want to change the GitHub settings to not require the nightly build in order to merge a PR, so that compiler regressions like this one don't block merging?

@hawkw hawkw merged commit db43c07 into master Feb 11, 2022
@hawkw hawkw deleted the eliza/clippy branch February 11, 2022 19:11
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.

None yet

2 participants