Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply rustfmt and fix Clippy warnings #1448

Merged
merged 1 commit into from
May 9, 2024
Merged

Apply rustfmt and fix Clippy warnings #1448

merged 1 commit into from
May 9, 2024

Conversation

newpavlov
Copy link
Member

Closes #1429

@@ -50,6 +50,7 @@
#![doc(test(attr(allow(unused_variables), deny(warnings))))]
#![no_std]
#![cfg_attr(feature = "simd_support", feature(portable_simd))]
#![allow(unexpected_cfgs)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed after renaming doc_cfg to docsrs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably will leave fixing it for a later PR. We also should consider using doc_auto_cfg (it has certain issues, but I am not sure they affect us).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave it to a later PR anyway?

@@ -387,6 +379,7 @@ pub trait Weight: Clone {
/// - `Result::Err`: Returns an error when `Self` cannot represent the
/// result of `self + v` (i.e. overflow). The value of `self` should be
/// discarded.
#[allow(clippy::result_unit_err)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we introduce a separate error type as suggested by this lint?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to a new PR please

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean leave the allow for now and introduce new error type in a later PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Given how much needs reformatting, it should be in a dedicated PR (not mixed up with Clippy / other fixes).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so will be REALLY annoying to untangle and I would strongly prefer to do both formatting and fixing of Clippy lints in one PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attr still shouldn't be here. Let Clippy complain in the CI if necessary.

Copy link
Member Author

@newpavlov newpavlov May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would fail the CI job. Are you ready to merge a PR with a failing CI job? What is the reason to leave it for a later PR? If it's just to make 2 "clear" commits in the git history instead of just one, I don't think it's worth the associated trouble.

src/seq/index.rs Show resolved Hide resolved
@newpavlov
Copy link
Member Author

I suggest to also remove rustfmt.toml. Arguably, improvements in formatting are relatively minor (and even that can be debatable in certain cases) and it's more advantageous to adhere to the standard config. For particularly bad cases we can use #[rustfmt::skip].

@dhardy
Copy link
Member

dhardy commented May 8, 2024

I suggest to also remove rustfmt.toml

It certainly needs reviewing — removing is acceptable.

rand_chacha/src/guts.rs Show resolved Hide resolved
rand_distr/src/gamma.rs Outdated Show resolved Hide resolved
rand_distr/src/hypergeometric.rs Outdated Show resolved Hide resolved
rand_distr/src/weighted_tree.rs Outdated Show resolved Hide resolved
rand_distr/tests/value_stability.rs Outdated Show resolved Hide resolved
@@ -387,6 +379,7 @@ pub trait Weight: Clone {
/// - `Result::Err`: Returns an error when `Self` cannot represent the
/// result of `self + v` (i.e. overflow). The value of `self` should be
/// discarded.
#[allow(clippy::result_unit_err)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to a new PR please

src/seq/index.rs Show resolved Hide resolved
benches/benches/generators.rs Show resolved Hide resolved
rand_core/src/lib.rs Show resolved Hide resolved
rand_distr/src/hypergeometric.rs Outdated Show resolved Hide resolved
rand_distr/src/lib.rs Show resolved Hide resolved
@@ -50,6 +50,7 @@
#![doc(test(attr(allow(unused_variables), deny(warnings))))]
#![no_std]
#![cfg_attr(feature = "simd_support", feature(portable_simd))]
#![allow(unexpected_cfgs)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave it to a later PR anyway?

src/seq/index.rs Outdated Show resolved Hide resolved
@dhardy dhardy merged commit 1b762b2 into master May 9, 2024
15 checks passed
@dhardy dhardy deleted the clippy_fmt branch May 9, 2024 06:50
@dhardy dhardy mentioned this pull request Jul 10, 2024
23 tasks
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.

Task: rustfmt
2 participants