Clean up warnings #1444
Clean up warnings #1444
Conversation
Better to delete the deriving clauses altogether
| lookup (StringTable bs tbl) str = binarySearch 0 (topBound-1) (BS.pack str) | ||
| where | ||
| (0, topBound) = A.bounds tbl | ||
| (_assumedZero, topBound) = A.bounds tbl |
There was a problem hiding this comment.
I don't think this is an improvement, since the algorithm now runs further if the _assumedZero isn't actually zero. But should it run further? It's basically the question of whether you want asserts in prod or not.
Personally, I think partiality is better than an incorrect result. Even though the compiler warns for partiality, but it doesn't warn for incorrectness. Of course, it would be even better to prove the algorithm correct. But that isn't always feasible, and the lack of dependent types make it extra difficult.
There was a problem hiding this comment.
That's fair. How about:
- An explicit
case - With a call to
errorin the non-zero lower bound case (with a description of the violated invariant) - A
HasCallStackconstraint
| cpiToJSON :: [CandPkgInfo] -> Value | ||
| cpiToJSON [] = Null -- should never happen | ||
| cpiToJSON pkgs = object | ||
| cpiToJSON pkgs@(pkg:_) = object |
There was a problem hiding this comment.
This change is good! It makes the function total, the type system recognizes the complete pattern match, and readability is not impaired.
| ('a':what1@('d':'d':'e':'d':_)) -> 'A' : what1 | ||
| ('r':what1@('e':'m':'o':'v':'e':'d':_)) -> 'R' : what1 | ||
| _ -> "Changed " ++ what0 |
There was a problem hiding this comment.
This one I like, as readability is hardly impaired.
| | pkgs <- groupBy ((==) `on` (packageName . fst)) | ||
| (cmToList perPkg :: [(PackageId, Int)]) | ||
| , let pkgname = packageName (fst (head pkgs)) | ||
| | pkgs@((pkgId, _):_) <- groupBy ((==) `on` (packageName . fst)) | ||
| (cmToList perPkg :: [(PackageId, Int)]) | ||
| , let pkgname = packageName pkgId |
There was a problem hiding this comment.
Given that this is a list comprehension, a failing pattern will cause the element to be skipped. Previously, it would have been a crash. See my other comment for my reasoning of why crashes are better than incorrect results. So I don't think we should do this right now in this PR.
There was a problem hiding this comment.
Well, we're leaning on the guarantee that the sublists returned by groupBy are non-empty, per the docs of groupBy and group.
There's the alternative approach of using Data.List.NonEmpty.groupBy which we certainly could take here.
There was a problem hiding this comment.
That seems like it would be better since it has a more exact type!
|
I should have explained the value I see in addressing these warnings. I find Having said that, I know from experience that reviewing dozens of small refactors is not pleasant, and perhaps others don't share my outlook regarding warnings. Do we see value in aiming for near-zero warnings? |
| -- TODO bug? Maybe something to do with #1439 | ||
| _pkgid1 <- packageTarballInPath dpath | ||
| pkgid2 <- packageInPath dpath | ||
| guard (pkgVersion pkgid2 == pkgVersion pkgid2) |
There was a problem hiding this comment.
This is the sort of bogus code warnings are supposed to catch. I haven't yet learned enough to understand what is going on here, but surely it can't be right to compare pkgVersion pkgid2 with itself.
2 participants
hackage-serverbuilds with lots of warnings. They mostly fall in a few categories:Typeableinstancesheadandtailwhich warn in recent versions of GHC andbaseIs a PR to clean up all the warnings welcome?