search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

Disallow compile warnings in CI #720

Closed mightyiam closed 1 year ago

mightyiam commented 1 year ago

Co-authored-by: Shahar "Dawn" Or mightyiampresence@gmail.com

mightyiam commented 1 year ago

I see that CI is waiting on approval from a maintainer. So we will explain ourselves. Our intention in this PR is to disallow compile warnings in the codebase. And, of course, to eventually "take care" of each existing warning. The reason that this draft PR currently only attempts to make CI fail on compile warnings and does not yet fix the individual warnings is that we wanted to see it fail, first.

The warnings are one of:

In the cases of the former kind, we will use fully qualified paths to disambiguate, thereby eliminating the warning.

In the cases of the latter kind, we can place #[allow(deprecated)] to avoid the warnings. This would make sense, because the deprecated items are our own.

If maintainers agree with this intention of not having warnings in the codebase, consider allowing the CI run. And we will attempt to fix all warnings in reasonable ways.

phimuemue commented 1 year ago

Hi there, thanks for this.

Do you happen to have the fix for the warnings at hand? If so, could you open a PR?

mightyiam commented 1 year ago

@phimuemue sorry for taking so long. We do intend to work on this. @warren2k and I. If no one else intends to pick this up, we would appreciate the patience. Also, will our subsequent pull requests CI runs require approval or is it "first contribution"?

mightyiam commented 1 year ago

@phimuemue ready!

Philippe-Cholet commented 1 year ago

I'm no maintainer here but I would surely appreciate to see no warning while experimenting on Itertools.

~I'm not sure to understand why u32 is there in the first place, but why put it behind the "use_alloc" feature?~ Obviously only used in alloc context (Powerset).

Philippe-Cholet commented 1 year ago

Note that I just removed pow_scalar_base in https://github.com/rust-itertools/itertools/pull/735/commits/467134594c41f61a296f16b3aa3d41200c5e62b7 while updating Powerset.

mightyiam commented 1 year ago

Oh, that's nice. Thank you for your work, @Philippe-Cholet. And thank you for letting us know. We're working on this in mob programming format. We've made progress today. It's not in this branch yet. Another session scheduled for tomorrow.

Hey, do you suppose we could get exempt from workflows needing approval, perhaps?

Philippe-Cholet commented 1 year ago

I just wanted to let you know about the little upcoming conflict. As a non-maintainer, I don't know about workflows approval. From what I understand, you need it (from a maintainer) in your first PR and I guess it's automatic in future PR.

mightyiam commented 1 year ago

@phimuemue it seems done. Should work. So un-drafting. Please approve workflow.

mightyiam commented 1 year ago

Converted to draft because requires rebase.

mightyiam commented 1 year ago

Please approve workflow and review.

mightyiam commented 1 year ago

All checks passed!

warren2k commented 1 year ago

Rebased. Have yet to address the feedback.

jswrenn commented 1 year ago

I attempted to fix a merge conflict so I could merge this, but ultimately broke CI. I'm not sure why.

Could you roll back to 352bd2d, fix the merge conflict, and re-request my review?

mightyiam commented 1 year ago

Sure. Likely this weekend.

warren2k commented 1 year ago

All checks have passed. We think this is ready for merge.

mightyiam commented 1 year ago

Yes. Even though these two commits are included in #740 , they already provide value and so we'd love for them to be merged :wink: @jswrenn .