add rustdoc and clippy to github workflow

[hellow554]

This commit is quite a mouth full and I'm can understand if you want me to split this into smaller chunks.

Let me start explain it commit by commit.

First I created a clippy toml with the msrv set to 1.36.0, which is the version used in the github workflow.

Next I either fixed all the linted lines, or added a allow with a comment to it.
I did the same for all the tests.

Next I saw, that there is currently a rustfmt which skips the complete source base, based on a commit two years ago: https://github.com/rust-itertools/itertools/commit/641671e8b8b8737a38039d572022e38b9b43b8d5

"Temporarily disable" 😉

I then reformatted the whole codebase, which might break some PRs but they should be able to rebase it but just using cargo fmt easily.

Next, I fixed two warnings. One stray ``` and the other one being changed in https://github.com/rust-lang/rust/pull/96676 where you have to specify a macro link.

Last, but not least, I added rustfmt rustdoc and clippy to the ci.yml to enable them by default on PRs.

---

Feel free to comment whatever you think. I try to answer or change it :)

Thanks for the awesome crate :)

[hellow554]

So this exploded a bit. It went from 5 to 31 commits, but yeah, it may be easier for you to review, I can understand that.

I removed the fmt part completly and will open a different PR, so no worries about that anymore.

[hellow554]

I try to resolve the issues.

I see some concerns and I think thats ok. Everything you think is not worth changing or might be bad can be annotated by #[allow(clippy::xxx)] along with a comment.
That's totally fine, really.

I'm not a perfect rust developer, neither is clippy perfect at detecting whether a change is good or bad.
But I think, clippy suggests a lot of useful lints worth checking at least. Let's try to solve this one together :)

[phimuemue]

Hi. I took the liberty to merge the first batch of this. (See https://github.com/rust-itertools/itertools/pull/624 - sorry if this was a stupid idea, I'm a github noob.)

Could you rebase so that this PR is updated, and it's clear what remains to be reviewed?

[hellow554]

I think that was a good idea, so no worries :)

Rebased.

I think at least https://github.com/rust-itertools/itertools/pull/618/commits/bf2c114c859023c16ead2d5ebb77d5063c4a91f9 should be pulled in as well, but we can do that later. Without it you may get a lot of false clippy suggestions.

[phimuemue]

Hi again. If you still want to tackle this: Could you rebase and sqash the fixup into the original commits to avoid back-and-forth changes (and thus, simplify review)? And could you leave the unwrap_or_else in place, and just allow the lint?

[hellow554]

Done :)

I very much thank you for your patience and effort you put into this! It's not easy to review this kind of stuff. Many, many thanks!

Btw: I found many false-positives while using clippy, which helps all users, so many thanks for the many test cases ;)

[phimuemue]

Done :)

I very much thank you for your patience and effort you put into this! It's not easy to review this kind of stuff. Many, many thanks!

Btw: I found many false-positives while using clippy, which helps all users, so many thanks for the many test cases ;)

My pleasure. I merged the third batch in https://github.com/rust-itertools/itertools/pull/630.

[hellow554]

Rebased again. Take your time, I'm not in a hurry

[danieleades]

The MSRV should be set in Cargo.toml these days, using the rust-version key.

see https://github.com/rust-itertools/itertools/pull/672

[Philippe-Cholet]

I come here just after we added clippy to our CI (and rustfmt a while back). I did not know this PR but it has good content I agree with as I made some changes lately that were part of this without knowing it. Sadly, this was a too big PR to be merged in a reasonable short time and eventually got forgotten. Smaller focused PRs are better, I learnt that myself. I see it was a good discussion though.

I think we might want to salvage part of this (in other smaller PRs and close this one). About documentation maybe (RUSTDOCFLAGS="-Dwarnings" cargo doc --all-features) and resolve part of what clippy --all-targets --all tell us. Am I forgotting something here?

[Philippe-Cholet]

I first read your commits too fast as you were fixing clippy lints of lint groups (such as "pedantic/nursery") that our new CI job does not consider (and probably should not to not burden new contributors too much?).

I'm not gonna close this, it is a bit outdated but still valuable to cherry pick and update in more atomic PRs.

[hellow554]

I rebased the work to the latest master. I'm very happy with your decision to leave this open and maybe cherry pick the things you like. That's totally fine to me. Feel free to browse the commits.

[Philippe-Cholet]

I kept find_position improvement (merged), Eq implementation for MinMaxResult (merged) and two annoying lints (PR sent), I'm gonna stop here though.

The only remaining question for me here before closing is about adding or not RUSTDOCFLAGS="-Dwarnings" cargo doc --all-features to the CI. I'm not familiar with cargo doc much yet. What kind of error would it prevent: wrong links? bad formatting?

I'm in favor of rejecting wrong documentation, as the result is user-facing and should be correct. - @phimuemue

[hellow554]

What kind of error would it prevent: wrong links?

Correct. For examples of what this denies see https://doc.rust-lang.org/rustdoc/lints.html

Bad formatting, no: that's what rustfmt is for.

[Philippe-Cholet]

I was thinking wrong markdown text formatting such as This is **bold* text. but fair enough, thanks for the insightful link.

[jswrenn]

We should absolutely deny warnings in CI!

[Philippe-Cholet]

I don't know to how many PRs this one lead to but it surely was a great base, thank you.

[danieleades]

We should absolutely deny warnings in CI!

Only if the version of the linter in CI is pinned. It isn't currently.

Otherwise you can introduce breakages in contributor PRs that have nothing to do with the changes in the PR.

This should be done regardless of whether warnings are denied, in my opinion.

[Philippe-Cholet]

@danieleades It sure seems a good idea (even if it should not be a lot of breakages). In CI, instead of "stable", use a pinned version such as 1.75.0 and update it from time to time? I guess dependabot should be able to help us. However, #758 wanted us to drop 1.43.1 (our msrv) so it's not that helping yet.

[danieleades]

@danieleades It sure seems a good idea (even if it should not be a lot of breakages). In CI, instead of "stable", use a pinned version such as 1.75.0 and update it from time to time? I guess dependabot should be able to help us. However, #758 wanted us to drop 1.43.1 (our msrv) so it's not that helping yet.

I'd suggest pinning to a recent nightly version of clippy. The 'rust-version' (MSRV) config will prevent clippy from suggesting incompatible changes

unfortunately dependabot won't be able to automatically update a nightly version

[Philippe-Cholet]

Clippy Doc recommands stable in our case for maximum compatibility. And I personally prefer that contributors can run the same clippy as the CI one on their computer without having to install nightly.

https://github.com/rust-itertools/itertools/pull/740#issuecomment-1751878550 agrees about possible (rare) breakage.

@jswrenn @phimuemue @mightyiam Should we pin clippy version to a (still stable IMO) recent version of Rust (and therefore maintain it) to avoid eventual breakage with contributor PRs?

[mightyiam]

Once a while, a new stable rust toolchain is released, along with some new clippy lints, usually. Some of them are on by default. And some of those may trigger on our code. Usually, they are easy to resolve. And if they're not, we can allow on the item. Using "stable" rolling clippy means that in those cases, development will be blocked until they are resolved. That should motivate whomever wishes to see their PR merged to resolve the lint in a prior PR. Then, rebasing is easy. There could even be a button for it in PR pages (that's an option in the repo settings). Therefore I vote rolling stable clippy.

[Philippe-Cholet]

(As a member,) I have access to a "rebase" button. I also (for now) would choose small rare blocks over manual pin maintenance. We can still change our mind later. And if there was a big block, we can still temporarily pin it in the CI to resolve the block later.

[danieleades]

actually i think if you use the dtolnay toolchain action to pin to a stable version dependabot will create PRs to bump the version. That gives you the best of both worlds- linting is pinned and consistent across PRs, and dependabot automagically creates PRs which highlight any new lints or changes to be addressed as they are released.

[mightyiam]

That sounds good, as well.

[danieleades]

That sounds good, as well.

see #846