-
Notifications
You must be signed in to change notification settings - Fork 107
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
Fix "insufficient coverage" message #521
Conversation
- Fix a typo - Remove redundant `TestCount`
@@ -323,7 +323,7 @@ checkReport cfg size0 seed0 test0 updateUI = do | |||
successReport | |||
else | |||
failureReport $ | |||
"Labels not sufficently covered after " <> show tests <> " tests" | |||
"Labels not sufficiently covered after " <> show tests <> " tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before: Labels not sufficently covered after TestCount 100 tests
after: Labels not sufficiently covered after 100 tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
|
||
instance Show TestCount where | ||
showsPrec p (TestCount n) = | ||
showsPrec p n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We probably would want this for the other newtypes around
Int
as well. - Are ok with dropping support for GHC
8.0.*
? If yes, I can simplify this to:deriving Lift deriving newtype (Eq, Ord, Show, Num, Enum, Real, Integral)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on Show
👍
How about planning for a release this month, and after doing the release drop support for GHC 8.0.*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding GHC 8.0.*, anything works for me. If we are not ready to drop support now, then I guess we want that code as this. So from my perspective, this is ready to merge.
Regarding Show
instances of other newtype wraps, we might still want to add the manual instances or wait for after the release. Again, both works for me, but if now, I would do it as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, the best would be to merge this as-is, add Show
instances of other newtypes as separate PRs if needed, and drop GHC 8.0.* after this release. If you agree, I think we can merge this as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
|
||
instance Show TestCount where | ||
showsPrec p (TestCount n) = | ||
showsPrec p n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on Show
👍
How about planning for a release this month, and after doing the release drop support for GHC 8.0.*
?
@@ -323,7 +323,7 @@ checkReport cfg size0 seed0 test0 updateUI = do | |||
successReport | |||
else | |||
failureReport $ | |||
"Labels not sufficently covered after " <> show tests <> " tests" | |||
"Labels not sufficiently covered after " <> show tests <> " tests" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
Thank you @sol 🙏 |
Thanks @moodmosaic 🙏 |
TestCount