add rustdoc and clippy to github workflow #618

hellow554 · 2022-06-02T12:12:54Z

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: 641671e

"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 rust-lang/rust#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 :)

phimuemue

Wohoo, there's some really nice improvements in there! Maybe you can rebase against the most recent git, as some of the things should already be fixed.

In addition, may I ask you for a favor that would imho simplify reviewing quite a bit:

Taking up your idea: Could you actually split all the lints into separate commits (one commit per fixed lint, possibly even separated for tests/non-tests)? Sure, it'd be more commits, but it avoids the constant context switching. In addition, we could theoretically cherry-pick the ones that are already good-to-go, while leaving others for later.
Possibly postpone the cargo fmt-related stuff to an own PR (after all the other fixes arrived) - or at least do it as the last commit so the first steps could be cherry-picked.

We once had the discussion cargo fmting everything, and we plan to do it eventually, but it would probably break virtually all existing PRs - something we do not want to do lightly.

In addition, I am still reluctant to reject PRs that do not adhere to cargo fmt: I think it would be unfortunate if a contributor would have to rework stuff because of a x+1 vs x + 1. However, I'd still be happy about an automated solution that would commit the cargo fmt as a PR automatically if it observes that it is required. (See format all rust files #421 for more discussion.)

src/lib.rs

src/ziptuple.rs

src/adaptors/mod.rs

src/groupbylazy.rs

tests/quick.rs

.github/workflows/ci.yml

.rustfmt.toml

hellow554 · 2022-06-03T07:46:26Z

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.

phimuemue

Cool! Thanks for your effort, it's appreciated.

I commented the places that jumped to my eye. (All others look good to me.) Maybe you could dispel my concerns about the few remaining bits.

src/lib.rs

src/adaptors/mod.rs

src/multipeek_impl.rs

phimuemue · 2022-06-03T19:19:57Z

src/tuple_impl.rs

-                  .unwrap_or_else(|| buffer.len())
+                  .unwrap_or(buffer.len())


Out of curiosity: Was this suggested by clippy? If not, I'd stay with the original

Yes: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations

But there's an open issue for that, because I find that strange too: rust-lang/rust-clippy#8109

I could not find authoritative reference that calling len is guaranteed to only read a field, so please revert this to unwrap_or_else, and possibly add an explicit allow.

phimuemue · 2022-06-03T19:26:22Z

tests/quick.rs

-            if it.next().is_none() {
-                panic!("Iterator shouldn't be finished, may not be deterministic");
-            }
+            assert!(it.next().is_some(), "Iterator shouldn't be finished, may not be deterministic");


Thinking about this a second time raises the question: Is it problematic that it.next appears within the assert? (In my ideal world, an assert should be side-effect free.)

I can see your concern. In my world, side effects in assert is ok, in debug_assert it's bad (and there's a lint for that (written by me): https://rust-lang.github.io/rust-clippy/master/index.html#debug_assert_with_mut_call)

But I would understand if you would like to have that different. Please tell me.

Contrary to my initial giving in: Let's strive to keep asserts side-effect free. (A correct program should work even if someone decides to redefine assert to a no-op.)

phimuemue · 2022-06-03T19:27:45Z

tests/test_core.rs

-    #[derive(PartialEq, Debug)]
+    #[derive(PartialEq, Eq, Debug)]


Depends. Might Foo be explicitly used to test stuff that only relies on PartialEq - and not Eq?

https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq

Fine for me.

phimuemue · 2022-06-03T19:29:39Z

.github/workflows/ci.yml

+      - run: RUSTDOCFLAGS="-Dwarnings" cargo doc --all-features
+      - run: cargo clippy --all -- -Dwarnings


Could you quickly elaborate what would happen if someone pushes something that does not adhere to cargo clippy ? Would this PR be rejected?

Yes, it would be rejected. Same goes for cargo doc (and cargo fmt in the future as well).

Isn't that the point of having a CI, that the code is equally good (or bad)?

tl;dr

I'd like to have both a low bar for (especially new) contributors and try to acchieve a certain quality in the code base. Thus:

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

I'm unsure if rejecting PRs with lint problems is a good idea.

I'm very skeptical to reject PRs because of formatting.

cargo clippy

I assume the codebase will quickly converge to a lint-free state (especially after your changes are merged). Once in that state, avoiding new lint problems should be easy.

From my personal experience, I found it demotivating if a PR is rejected because of a clippy-false-positive (or one that does not matter in the circumstances at hand). I'd like to avoid this situation as far as possible.

For one-off contributors, requesting and waiting for changes might be just as much effort as just merging and fixing the lint, possibly indicating that clippy should only warn instead of reject the PR.

I assume frequent contributors do not introduce many new lints. (They may have clippy running on their local machines, so are aware of lint problems from the start.)

Generally, I think I'd prefer to only issue a warning (thus requiring human interaction to resolve it) instead of plainly rejecting the PR, assuming that it does not generate too much work on the contributors and the maintainers. I would be open to have clippy reject PRs if it's easier/better to implement/maintain/work with/others really want it/etc, as it sometimes gives really useful advice.

cargo fmt

I guess the main advantage of having a cargo fmtd code base is that contributors have an idea how our code is written. However, for this it should be enough to "cargo fmt from time to time" (read: after a successfully merged PR). As such, I'd prefer not to reject PRs because of formatting problems (again to avoid frustration) - and instead have the formatting problems fixed automatically by our CI.

I'm on your side with clippy. I pushed the change just now.

As for formatting, I'm somewhat against your idea. The burden of a contributor is to run cargo fmt prior to git push, so that should be somewhat pain free.

I can understand your point of view that some people just do "a line change", open a PR and never come back again, so you have a failing CI.
But, IIRC you, as a maintainer, can still merge that, despite the failing CI and "fix it later", i.e. after the merge. Alternativly you can always push changes to the branch, when the opener has the tick Allow edits and access to secrets by maintainers which is the default when opening a PR.

But let's talk about cargo fmt in #619 as it's no longer part of this PR.

src/unique_impl.rs

hellow554 · 2022-06-07T07:10:29Z

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 :)

624: Hellow554 lints 1 r=phimuemue a=phimuemue First batch of #618. bors r+ Co-authored-by: Marcel Hellwig <[email protected]>

phimuemue · 2022-06-08T16:07:17Z

Hi. I took the liberty to merge the first batch of this. (See #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 · 2022-06-09T05:44:55Z

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

Rebased.

I think at least bf2c114 should be pulled in as well, but we can do that later. Without it you may get a lot of false clippy suggestions.

627: Hellow554 lints 2 r=phimuemue a=phimuemue Second batch of #618. Co-authored-by: Marcel Hellwig <[email protected]>

phimuemue

I took the freedom to merge a second batch of this.

@hellow554 Thank you! If you find time, could you let us know if you want to further push things forward on this PR?

phimuemue · 2022-06-24T15:15:13Z

clippy.toml

@@ -0,0 +1 @@
+msrv = "1.36.0"


Do we need some remark that this line has to be updated along other files? Apart from that, I'm fine with this.

It depends. I can't find any word about the msrv, apart from the github workflow, so first step would be to mention it in the readme I guess?
Second: there's the rust-version field in the Cargo.toml file that can be used as well. A clippy PR is pending to parse that field in addition to the clippy.toml file: rust-lang/rust-clippy#8774

As soon as that lands, the clippy.toml can be deleted again, so I don't think that that should be documented? 😅

tests/quick.rs

phimuemue · 2022-06-24T15:38:49Z

tests/quick.rs

-            if it.next().is_none() {
-                panic!("Iterator shouldn't be finished, may not be deterministic");
-            }
+            assert!(it.next().is_some(), "Iterator shouldn't be finished, may not be deterministic");


Contrary to my initial giving in: Let's strive to keep asserts side-effect free. (A correct program should work even if someone decides to redefine assert to a no-op.)

phimuemue · 2022-07-06T21:10:14Z

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 · 2022-07-07T09:16:27Z

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 ;)

630: Hellow554 lints 3 r=phimuemue a=phimuemue Next batch of #618. bors r+ Co-authored-by: Marcel Hellwig <[email protected]>

phimuemue · 2022-07-07T20:34:56Z

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 #630.

hellow554 · 2022-07-07T20:47:03Z

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

hellow554 · 2024-01-10T12:49:55Z

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 · 2024-01-10T15:34:07Z

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 · 2024-01-10T15:55:16Z

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 · 2024-01-10T16:02:58Z

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

jswrenn · 2024-01-10T16:04:08Z

We should absolutely deny warnings in CI!

Philippe-Cholet · 2024-01-10T18:46:48Z

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

danieleades · 2024-01-11T00:43:01Z

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 · 2024-01-11T08:36:04Z

@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 · 2024-01-11T09:01:46Z

@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 · 2024-01-11T09:38:11Z

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.

#740 (comment) 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 · 2024-01-12T03:04:43Z

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 · 2024-01-12T07:34:16Z

(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 · 2024-01-12T07:48:06Z

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 · 2024-01-12T08:40:18Z

That sounds good, as well.

danieleades · 2024-01-13T03:55:13Z

That sounds good, as well.

see #846

phimuemue reviewed Jun 2, 2022

View reviewed changes

hellow554 force-pushed the clippy-lints branch from c5cc24c to 502b284 Compare June 3, 2022 07:44

hellow554 force-pushed the clippy-lints branch from 502b284 to 873dcf4 Compare June 3, 2022 07:48

hellow554 changed the title ~~add rustfmt, rustdoc and clippy to github workflow~~ add rustdoc and clippy to github workflow Jun 3, 2022

phimuemue reviewed Jun 3, 2022

View reviewed changes

scottmcm reviewed Jun 3, 2022

View reviewed changes

src/unique_impl.rs Outdated Show resolved Hide resolved

hellow554 mentioned this pull request Jun 7, 2022

Alternatively, you could consider having Positions store an iter::Enumerate<I> instead of I+usize. #622

Closed

phimuemue mentioned this pull request Jun 8, 2022

Hellow554 lints 1 #624

Merged

bors bot added a commit that referenced this pull request Jun 8, 2022

Merge #624

5c11e00

624: Hellow554 lints 1 r=phimuemue a=phimuemue First batch of #618. bors r+ Co-authored-by: Marcel Hellwig <[email protected]>

hellow554 force-pushed the clippy-lints branch from 9dd1ad4 to af3ec56 Compare June 9, 2022 05:43

phimuemue mentioned this pull request Jun 24, 2022

Hellow554 lints 2 #627

Merged

bors bot added a commit that referenced this pull request Jun 24, 2022

Merge #627

5459a26

627: Hellow554 lints 2 r=phimuemue a=phimuemue Second batch of #618. Co-authored-by: Marcel Hellwig <[email protected]>

phimuemue reviewed Jun 24, 2022

View reviewed changes

hellow554 force-pushed the clippy-lints branch 2 times, most recently from cfeed52 to 8f411a4 Compare June 27, 2022 06:39

hellow554 force-pushed the clippy-lints branch 2 times, most recently from 74bcfe4 to 986992a Compare July 7, 2022 09:15

phimuemue mentioned this pull request Jul 7, 2022

Hellow554 lints 3 #630

Merged

bors bot added a commit that referenced this pull request Jul 7, 2022

Merge #630

6b7063a

630: Hellow554 lints 3 r=phimuemue a=phimuemue Next batch of #618. bors r+ Co-authored-by: Marcel Hellwig <[email protected]>

hellow554 force-pushed the clippy-lints branch 2 times, most recently from 2e2dc50 to 071aa68 Compare July 7, 2022 20:46

phimuemue mentioned this pull request Sep 7, 2022

Should we release 0.10.4? #640

Merged

hellow554 added 3 commits January 10, 2024 13:19

fix clippy::if_not_else

e663b7a

fix clippy::enum_glob_use

c69ef49

fix clippy::option_if_let_else

bb04042

hellow554 force-pushed the clippy-lints branch 2 times, most recently from afec10a to cf4e2ae Compare January 10, 2024 12:44

hellow554 added 3 commits January 10, 2024 13:48

fix some more clippy lints

ae943da

run rustfmt

09effd5

fix clippy::use_self

ab4bd08

hellow554 force-pushed the clippy-lints branch from cf4e2ae to ab4bd08 Compare January 10, 2024 12:48

This was referenced Jan 10, 2024

Faster find_position #837

Merged

Clippy style #839

Merged

MinMaxResult: derive Eq #838

Merged

Philippe-Cholet mentioned this pull request Jan 10, 2024

CI: add cargo doc #840

Merged

Philippe-Cholet closed this Jan 10, 2024

danieleades mentioned this pull request Jan 13, 2024

pin the toolchain version used by clippy #846

Closed

		- run: RUSTDOCFLAGS="-Dwarnings" cargo doc --all-features
		- run: cargo clippy --all -- -Dwarnings

add rustdoc and clippy to github workflow #618

add rustdoc and clippy to github workflow #618

Conversation

hellow554 commented Jun 2, 2022

phimuemue left a comment • edited Loading

Choose a reason for hiding this comment

hellow554 commented Jun 3, 2022

phimuemue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phimuemue Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phimuemue Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tl;dr

cargo clippy

cargo fmt

Choose a reason for hiding this comment

hellow554 commented Jun 7, 2022

phimuemue commented Jun 8, 2022

hellow554 commented Jun 9, 2022

phimuemue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phimuemue commented Jul 6, 2022

hellow554 commented Jul 7, 2022 • edited Loading

phimuemue commented Jul 7, 2022

hellow554 commented Jul 7, 2022

hellow554 commented Jan 10, 2024

Philippe-Cholet commented Jan 10, 2024

hellow554 commented Jan 10, 2024 • edited Loading

Philippe-Cholet commented Jan 10, 2024

jswrenn commented Jan 10, 2024

Philippe-Cholet commented Jan 10, 2024

danieleades commented Jan 11, 2024

Philippe-Cholet commented Jan 11, 2024 • edited Loading

danieleades commented Jan 11, 2024 • edited Loading

Philippe-Cholet commented Jan 11, 2024

mightyiam commented Jan 12, 2024

Philippe-Cholet commented Jan 12, 2024

danieleades commented Jan 12, 2024

mightyiam commented Jan 12, 2024

danieleades commented Jan 13, 2024

phimuemue left a comment •

edited

Loading

phimuemue Jun 7, 2022 •

edited

Loading

phimuemue Jun 3, 2022 •

edited

Loading

phimuemue left a comment •

edited

Loading

hellow554 commented Jul 7, 2022 •

edited

Loading

hellow554 commented Jan 10, 2024 •

edited

Loading

Philippe-Cholet commented Jan 11, 2024 •

edited

Loading

danieleades commented Jan 11, 2024 •

edited

Loading