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

Fix some clippy lints #443

Merged
merged 8 commits into from
Jan 27, 2024
Merged

Fix some clippy lints #443

merged 8 commits into from
Jan 27, 2024

Conversation

hellow554
Copy link
Contributor

I really love clippy and I ran it so see if anything can be improved.
But the very first things was an clippy error that I saw, so I looked at it and decided that that error is acceptable and can be allowed.

This PR is read best per commit.
If you like I can purge certain commits and/or split them into seperate PRs.

@iliekturtles
Copy link
Owner

Thanks for the PR! Did you run clippy with extra parameters to get the Self warnings? I kicked off tests and there are some errors, could you review?

@hellow554
Copy link
Contributor Author

with extra parameters to get the Self warnings

yes, indeed: cargo clippy -- -Wclippy::pedantic -Wclippy::nursery

I'll look into the CI errors, give me a minute :)

@hellow554
Copy link
Contributor Author

Yeah... I was not sure about the unit field in that struct.
I thought it was a too big issue to remove it, but I can give it a try

@hellow554
Copy link
Contributor Author

I did something, it works on my machine, let's hope it gets through CI

@hellow554
Copy link
Contributor Author

package memchr v2.6.4 cannot be built because it requires rustc 1.61 or newer, while the currently active rustc version is 1.60.0

:( Not my fault, but I can try to fix it, if you like

@iliekturtles
Copy link
Owner

Sorry for the extended delay! Go ahead and add a commit to bump the MSRV to 1.61.0. You can model the commit after 8640730.

@iliekturtles
Copy link
Owner

I just realized the 1.61.0 requirement is causing a problem in master right now and isn't from this PR. The MSRV bump should be separate if you get a chance to submit a new PR before I can get it fixed.

@hellow554
Copy link
Contributor Author

@iliekturtles I opened #456 Please have a look :)

Without this, `clippy` would give a lot of errors, because of the
`prefix!(kilo) / prefix!(kilo)` and other similar macro calls which have
result in 1 which is okay for our use case. Therefore the
`#[allow(...)]` seems reasonable here.
This fixes another `clippy` warning:

```
warning: non-canonical implementation of `clone` on a `Copy` type
    --> src/system.rs:1468:41
     |
1468 |                   fn clone(&self) -> Self {
     |  _________________________________________^
1469 | |                     Self {
1470 | |                         dimension: $crate::lib::marker::PhantomData,
1471 | |                         unit: self.unit.clone(),
1472 | |                         style: self.style.clone(),
1473 | |                     }
1474 | |                 }
     | |_________________^ help: change this to: `{ *self }`
```
The value of that type is never use, only the actual type parameter `N`
@iliekturtles iliekturtles merged commit 9abc39e into iliekturtles:master Jan 27, 2024
14 checks passed
@iliekturtles
Copy link
Owner

Thanks again for this PR. I just merged! I went through doing a final review. I made some minor edits to the commit messages and a couple fixes to the code.

@hellow554
Copy link
Contributor Author

Sure enough! Thanks for the merge

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.

2 participants