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

Resolve all lints and formating; split style checks into separate job #2147

Merged
merged 4 commits into from
Nov 4, 2023

Conversation

infogulch
Copy link
Contributor

This PR resolves all clippy lints and formats the codebase. It also splits the formatting and clippy CI checks into a separate job and makes them 'mandatory'; really this means that the clippy/fmt check may show as failed but the regular build-test jobs will continue as normal, and you can choose to merge with outstanding clippy/formatting issues if you want.

My goal here is to make clippy suggestions and autoformatting actually useful during development if you have it set up. It's very good at finding better ways to represent something for new and experienced rust devs alike, but it's not very useful if the suggestion you need is buried under a pile of 400+ other suggestions. Hence this PR: rip off the bandaid once so the suggestions become useful again.

I intentionally tried to avoid making behavioral changes, and our test suite backs us up here. To give a flavor of applied changes here are some common lints that were fixed:

  • Unnecessarily taking the address of or dereferencing a variable
  • Using if let construct instead of match for single match arms
  • Using matches!() macro instead of match when all match arms return true and the default arm returns false
  • Adding annotations to allow a clippy pattern in a particular location where it's reasonable or doesn't have a clear fix (you could see these as TODOs, just search for allow(clippy:: to see the list)
  • Using specialized methods instead of a method + expression
  • Removing elidable lifetimes
  • Using assert!() instead of assert_eq!(_, bool)
  • Switching to a simpler iteration method (from while to for etc)
  • Deriving the default trait instead of implementing it manually
  • Removing unnecessary return

This basically touches all rust files which could result in shadowing potentially useful historical context when viewing the history or blame of a file. To mitigate this issue I added the lint/formatting commit id to a new .git-blame-ignore-revs file which is a list of commits that both git and github.com can use to skip commits that make bulk changes to a repo. See GitHub docs.


There are a lot of changes here, and I understand that not coordinating a change like this ahead of time could make accepting this pr complicated and may require extra work to make ready to merge.

@mthom mthom merged commit da0018e into mthom:master Nov 4, 2023
12 checks passed
@infogulch infogulch deleted the lint-format branch November 4, 2023 21:23
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.

2 participants