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

address a bunch of pedantic clippy lints #106

Merged
merged 14 commits into from
Apr 9, 2023
Merged

Conversation

danieleades
Copy link
Contributor

these might not all be of interest, so i've split them into separate commits for ease of review

@danieleades danieleades requested a review from ttytm as a code owner April 9, 2023 07:54
@kevinmatthes
Copy link
Collaborator

I looked over your changes roughly and it looks great to me! I had been working on something similar, as well, but I did not have the time to upload it. I planned to add more restrictive Clippy lints, see #92, and the changes you submitted are a very good step towards that.

There is only one thing I would like to ask you to change: would you please change the lint level from "warn" to "deny"? In #92, the discussion was mostly about the "deny" level, so I think this would be a good enhancement.

@github-actions
Copy link

github-actions bot commented Apr 9, 2023

Test Results

  3 files  ±0    3 suites  ±0   13s ⏱️ ±0s
  9 tests ±0    9 ✔️ ±0  0 💤 ±0  0 ±0 
27 runs  ±0  27 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit bfd0c57. ± Comparison against base commit 6822751.

♻️ This comment has been updated with latest results.

@ttytm
Copy link
Owner

ttytm commented Apr 9, 2023

Thanks for the PR @danieleades!

Great to see that @kevinmatthes gives his go. #92 can then nicely expand on this work. I merged #107 already, if you could solve the conflicts this one will follow 👍.

src/main.rs Outdated Show resolved Hide resolved
@ttytm ttytm merged commit a02502a into ttytm:main Apr 9, 2023
@danieleades danieleades deleted the clippy branch April 10, 2023 06:26
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.

3 participants