Skip to content

Fix some clippy lints#443

Merged
iliekturtles merged 8 commits into
iliekturtles:masterfrom
hellow554:master
Jan 27, 2024
Merged

Fix some clippy lints#443
iliekturtles merged 8 commits into
iliekturtles:masterfrom
hellow554:master

Conversation

@hellow554

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@hellow554

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@iliekturtles

Copy link
Copy Markdown
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
Copy Markdown
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