clippy: allow result_large_err in some places#5990
clippy: allow result_large_err in some places#5990yihau wants to merge 4 commits intoanza-xyz:masterfrom
Conversation
brooksprumo
left a comment
There was a problem hiding this comment.
Can these annotations be applied on the individual offenders instead of the whole files?
|
yes we can. I thought about that before. since it will be hundreds of lines changes, I tried to use this lazy method to check if I can do less haha. happy to do it right now |
This reverts commit 04964c3.
Oh oof... That's no fun... If you have the clippy log that has all of them, can you upload/paste is here so I can get a sense for it? |
|
sure! here is the logs and it just a part of them 🫠 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5990 +/- ##
=======================================
Coverage 82.8% 82.8%
=======================================
Files 830 830
Lines 377256 377256
=======================================
+ Hits 312529 312596 +67
+ Misses 64727 64660 -67 🚀 New features to boost your workflow:
|
Great! Can you just annotate the |
|
no. I tried but no luck 🫠 |
|
I reverted the previous commit and use a new strategy for adding clippy annotation:
|
|
Wait, why didn't we see this clippy lint when upgrading stable rust? |
oh... we removed stable clippy in #1661. we only run nightly clippy atm |
🫠 Well... that doesn't seem too good. I think we should run clippy in stable again. We can get these lints resolved (and nightly rust updated) first though. |
I guess we should force ourselves to update the stable and nightly versions together so that we can ensure clippy issues are caught and save some ci time and yeah, let's focus on the clippy solving in this PR! |
Something seems strange. I don't understand why this wouldn't work. Most of the changes that I inspected were all for the ClientResult type. Do you have more information? |
| @@ -1,3 +1,4 @@ | |||
| #![allow(clippy::result_large_err)] | |||
There was a problem hiding this comment.
These crate-wide allows are red flags to me. I can't in good conscious sign off on the PR without understanding more why we can't more targeted annotations.
There was a problem hiding this comment.
I did this because it’s a cli crate. It ideally won’t affect our validator. If you feel it’s too broad, I can spend some time adding annotations to each one
I have no idea. I tried add the annotation to Result and Error but no one works. Maybe the attribute doesn’t get inherited by the function? You should be able to reproduce this by |
|
Let's only bump rust nightly to the latest version that matches our stable rust. If we do that, I think we don't need this PR. See #5952 (comment). |
|
Yes, we'll likely need to address this later, when upgrading to rust 1.88.0 though. I think a GH issue can be created and assigned to the owner of the ClientResult for them to determine the correct resolution. |
Problem
part of nightly rust bumping #5952
https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
I guess fixing this would cause some breaking changes. keep it as is and block newly created
Summary of Changes
allow in some places