Add clippy into CI and fix clippy warnings #1569
Conversation
ikrivosheev
force-pushed
the
feature/clippy
branch
2 times, most recently
from
ce94466 to
be0dff0
Compare
ikrivosheev
force-pushed
the
feature/clippy
branch
from
be0dff0 to
6ae1fd8
Compare
ikrivosheev
force-pushed
the
feature/clippy
branch
from
c19d9b0 to
6518388
Compare
|
Tests are failed on nightly and beta after rustc enable niche-filling optimization. Issue: rust-lang/rust#46213 |
| @@ -14,7 +14,7 @@ edition = "2018" | |||
| autoexamples = false | |||
|
|
|||
| # also update in README.md (badge and "Rust version requirements" section) | |||
| rust-version = "1.48" | |||
| rust-version = "1.50" | |||
There was a problem hiding this comment.
The MSRV bump shouldn't be neccesary once #1549 is merged.
There was a problem hiding this comment.
Well, you can see there is an error now: https://github.com/Geal/nom/actions/runs/3040066364
There was a problem hiding this comment.
This PR fix error and also add some improvements.
There was a problem hiding this comment.
Yes, and #1549 fixes that by using min() instead of clamp().
There was a problem hiding this comment.
Well, I don't see a problem to drop support for rust version which had a release 2 years ago.
There was a problem hiding this comment.
@ikrivosheev It is a huge problem, because it would require a major version bump. Quoting the README (emphasis in original):
The 7.0 series of nom supports Rustc version 1.48 or greater. It is known to work properly on Rust 1.41.1 but there is no guarantee it will stay the case through this major release.
The current policy is that this will only be updated in the next major nom release.
MSRV guarantees are serious business for widely used libraries. It's fine if your library doesn't commit to an MSRV (but that means no library that has an MSRV can use your library as a dependency), but if your library does make such a commitment, you cannot break it.
There was a problem hiding this comment.
Hopefully 8.0 changes the MSRV guarantee to minor compatibility breaks (8.x.0) rather than major (x.0). This is the approach of most libraries I work with and it seems to work out well. Having more MSRV smarts in cargo would he,lp though.
There was a problem hiding this comment.
MSRV will probably change for nom 8 yes, but will stay the same for nom 7. Contrary to some libraries that bump MSRV in minor versions, we can't do that for nom because those versions have to be shipped in software for distributions like debian
|
closing this in favor of #1585, thanks for the help though! |
Improvements
cargo clippyallow)CI example: https://github.com/ikrivosheev/nom/actions/runs/3344876947