Skip to content

Conversation

jbangelo
Copy link
Collaborator

@jbangelo jbangelo commented Sep 4, 2025

This sets the clippy::pedantic set of lints by default to be warnings, but remember that CI will fail on any warnings. The second commit goes through and fixes most of the new warnings. There are a handful of places where some of the lints are disabled.

Disabled lints

  • #[allow(clippy::many_single_char_names)] was used twice (crate::coords::llh::LLHRadians::to_ecef(), and crate::math::compile_time_sqrt()). These two functions are rather short and implement common mathematical formulas, so there aren't really super descriptive names to give the variables and doing so would make the implementation even more different than the original source.
  • #![allow(clippy::cast_possible_truncation, clippy::cast_sign_loss, clippy::cast_precision_loss)] for the entire crate::time::mjd module, basically we're doing a lot of not great casting
  • #![allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] for the entire crate::time::utc module, similar use of lots of not great casting

@jbangelo jbangelo force-pushed the jbangelo/clippy-pedantic branch from da9237d to fc1f6b7 Compare September 4, 2025 03:45
@jbangelo jbangelo force-pushed the jbangelo/clippy-pedantic branch from fc1f6b7 to 0ca157c Compare September 4, 2025 03:48
@jbangelo jbangelo marked this pull request as ready for review September 4, 2025 04:22
@jbangelo jbangelo requested a review from a team as a code owner September 4, 2025 04:22
@@ -57,30 +61,37 @@ impl UtcParams {
}

/// Modulo 1 sec offset from GPS to UTC \[s\]
#[must_use]

Choose a reason for hiding this comment

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

Decided to look up why this is not the default generally and looks like I stumbled upon another extremely controversial topic in the Rust community lol. Kind of insane they don't have a way of setting this for all functions in the cargo.toml or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. @tzilist has suggested to start enabling the Clippy pedantic lints on more stuff. I don't think we've spent enough time to figure out if there are lints under that category that we want to blanket ignore. I think we're all happy to hear feedback if there are ones you think are just too noisy or are overly strict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can enable just the must_use lint with #![warn(clippy::must_use_candidate)] or I'm sure there's a way either in the Cargo.toml or the clippy.toml as well.

Choose a reason for hiding this comment

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

Yeah no complaints just find it bizarre you need to cover all your functions with this attribute and there is no way to set it as a default for all functions and rather it be an opt out feature.

Copy link

@tzilist tzilist left a comment

Choose a reason for hiding this comment

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

Had a little nit, but going to approve regardless as it's quite small

@@ -128,6 +128,7 @@ impl LLHRadians {
///
/// Uses the [`WGS84`] Ellipsoid
#[must_use]
#[allow(clippy::many_single_char_names)] // It's math, whatyagonnado?
Copy link

Choose a reason for hiding this comment

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

I think we should use expect here (which will throw an error if someone comes in and fixes the lint, and thus, can be removed). You can also add reasons into both the expect and allow macros 😄

Suggested change
#[allow(clippy::many_single_char_names)] // It's math, whatyagonnado?
#[expect(clippy::many_single_char_names, reason = "It's math, whatyagonnado?")]

Copy link

Choose a reason for hiding this comment

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

nice i didn't know this existed!

@jbangelo jbangelo merged commit d2c0b1a into master Sep 9, 2025
6 checks passed
@jbangelo jbangelo deleted the jbangelo/clippy-pedantic branch September 9, 2025 00:46
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.

4 participants