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

House keeping #117

Closed
wants to merge 1 commit into from
Closed

House keeping #117

wants to merge 1 commit into from

Conversation

Alextopher
Copy link

  • Runs cargo fmt
  • Addresses all existing cargo clippy lints
  • Adds CI to verify cargo fmt and cargo clippy

There were a lot of lints in tests - to make my life easier I removed a lot of boilerplate.

Better late than never 😄

.and_hms_opt(hour, minute, second)
// .ymd(year as i32, month, day_of_month)
// .and_hms_opt(hour, minute, second)
.with_ymd_and_hms(
Copy link
Author

Choose a reason for hiding this comment

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

looks like this might interfere with #115

Copy link
Author

Choose a reason for hiding this comment

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

@zslayton #112 #115 #116 all look like quality bug fixes.

I think it would be best if those were merged first then I will rebase this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Two of these have been merged, #115 (the most substantive change) is still pending. #123 addressed the current clippy and rustfmt issues, but did not add those steps to the CI. If you're interested in adding the CI steps as a fresh PR, that might be the quickest path forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry I missed that this PR included the same fixes for fmt and clippy as I made in #123, sorry about that! But I didn't add any CI checks (which I should've) so this PR and changes still looks like a good one to merge!

Copy link
Author

Choose a reason for hiding this comment

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

I'll try to rebase this today!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Alextopher #129 just got merged. In case you are currently working on the rebase, I hope this message reaches you before another rebase causes you extra work.

@LeoniePhiline
Copy link
Contributor

How about adding the following clippy linting configuration to Cargo.toml:

[lints.clippy]
pedantic = { level = "warn", priority = -1 }
perf = { level = "warn", priority = -1 }
cargo = { level = "warn", priority = -1 }

I also like to use these code quality compiler lints in my own projects:

[lints.rust]
elided_lifetimes_in_paths = "warn"
future_incompatible = { level = "warn", priority = -1 }
let_underscore = { level = "warn", priority = -1 }
missing_copy_implementations = "warn"
missing_debug_implementations = "warn"
missing_docs = "warn"
# must_not_suspend = "warn" # Unstable - tracked at https://github.com/rust-lang/rust/issues/83310
non_ascii_idents = "warn"
nonstandard_style = { level = "warn", priority = -1 }
noop_method_call = "warn"
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage)'] }
unnameable_types = "warn"
unreachable_pub = "warn"
unused = { level = "warn", priority = -1 }
unused_crate_dependencies = "warn"
unused_lifetimes = "warn"

# lossy_provenance_casts = "deny" # Unstable - tracked at https://github.com/rust-lang/rust/issues/95228
# fuzzy_provenance_casts = "deny" # Unstable - tracked at https://github.com/rust-lang/rust/issues/95228
unsafe_code = "deny" # Exceptions must be discussed and deemed indispensable and use `#![deny(invalid_reference_casting, unsafe_op_in_unsafe_fn)]`.

@Alextopher
Copy link
Author

Hi everyone, I looked into it and it doesn't seem like there's much more in this PR that is worth merging. A new PR is probably better, as for now I think I should close this PR and leave it an open project for someone else. Thanks!

@Alextopher Alextopher closed this Nov 1, 2024
bombsimon added a commit to bombsimon/cron that referenced this pull request Dec 7, 2024
Followup from zslayton#117 and partly zslayton#123 addressing these issues. This is now
checked in the GitHub action pipeline to avoid adding new violations.
zslayton pushed a commit that referenced this pull request Dec 26, 2024
* chore: Add `cargo fmt` and `clippy` checks to GitHub action

Followup from #117 and partly #123 addressing these issues. This is now
checked in the GitHub action pipeline to avoid adding new violations.

* chore: Update `actions/checkout`

* fix: Fix `clippy` issues

Remove lifetime annotations which can rely on lifetime elision.
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