Lack of Show on many types hurts explorability of the codebase

[steve-chavez]

Say I want to explore some functions of the MediaType module with the cabal repl, the following will fail:

$ cabal v2-repl

ghci> import PostgREST.MediaType

ghci> decodeMediaType "application/json"

<interactive>:2:1: error:
    • No instance for (Show MediaType)
        arising from a use of ‘System.IO.print’
      There are instances for similar types:
        instance [safe] Show
                          http-media-0.8.0.0:Network.HTTP.Media.MediaType.Internal.MediaType
          -- Defined in ‘http-media-0.8.0.0:Network.HTTP.Media.MediaType.Internal’
    • In a stmt of an interactive GHCi command: System.IO.print it

If I add the required Show instances(manually, which turns into a chore quickly):

diff --git a/src/PostgREST/MediaType.hs b/src/PostgREST/MediaType.hs
index 39840a80..ebb5cad6 100644
--- a/src/PostgREST/MediaType.hs
+++ b/src/PostgREST/MediaType.hs
@@ -33,17 +33,17 @@ data MediaType
   | MTAny
   | MTOther ByteString
   | MTPlan MTPlanAttrs
-  deriving Eq
+  deriving (Show, Eq)
 
-data MTPlanAttrs = MTPlanAttrs (Maybe MediaType) MTPlanFormat [MTPlanOption]
+data MTPlanAttrs = MTPlanAttrs (Maybe MediaType) MTPlanFormat [MTPlanOption] deriving Show
 instance Eq MTPlanAttrs where
   MTPlanAttrs {} == MTPlanAttrs {} = True -- we don't care about the attributes when comparing two MTPlan media types
 
 data MTPlanOption
-  = PlanAnalyze | PlanVerbose | PlanSettings | PlanBuffers | PlanWAL
+  = PlanAnalyze | PlanVerbose | PlanSettings | PlanBuffers | PlanWAL deriving Show
 
 data MTPlanFormat
-  = PlanJSON | PlanText
+  = PlanJSON | PlanText deriving Show
 
 -- | Convert MediaType to a Content-Type HTTP Header
 toContentType :: MediaType -> Header

It will now succeed:

$ cabal v2-repl

ghci> import PostgREST.MediaType
ghci> decodeMediaType "application/json"
MTApplicationJSON

This is also a problem for debugging because one cannot do simple traceShow like previously discussed here #2523 (comment)

---

Code coverage is the main reason for removing those Shows. How can we include them again for all types and still pass the codecov checks?

cc @wolfgangwalther

[aljungberg]

I might be misunderstanding why this happens but codecov checks seem particularly weak with data. I guess the types are never "executed" per se so it doesn't trigger any line counts. Maybe there's some updated version of hpc-codeconv that's smarter about it?

Or maybe it's more pragmatic to just allow the code coverage to drop in those cases. The coverage isn't an end goal in itself, just an indicator, after all. Having more easily explorable and repl friendly code seems worth more to me.

One thing we discussed in the previous ticket was to simply have a separate file to derive show on everything. It'd have terrible coverage but everyone would know it's safe to ignore.

[steve-chavez]

Or maybe it's more pragmatic to just allow the code coverage to drop in those cases. The coverage isn't an end goal in itself, just an indicator, after all. Having more easily explorable and repl friendly code seems worth more to me.

Yeah, I agree.

I'm planning to do a PR that just adds all the Show instances and then open an issue to see how we can increase code coverage for those.

Ignore the above, Laurence got the idea on #2657.

[steve-chavez]

One thing we discussed in the previous ticket was to simply have a separate file to derive show on everything.

Seems that doesn't work according to #2657 (comment).

The other option is to just add the Show to the types directly.

[aljungberg]

Pity! I mean, if it were me, I'd just add it to every type everywhere then. Given the current limitations of hpc-codeconv, it's necessary to think of code coverage as a relative metric rather than an absolute number. Like we care if it goes up or down with individual PRs, but we know the absolute number has an irreducibly wrong component so we take it with a grain of salt.

[wolfgangwalther]

I might be misunderstanding why this happens but codecov checks seem particularly weak with data. I guess the types are never "executed" per se so it doesn't trigger any line counts. Maybe there's some updated version of hpc-codeconv that's smarter about it?

This is not about codecov. This is about hpc and our use of RecordWildCards (and related language extensions) - those are just not marked as "covered" by hpc. See https://gitlab.haskell.org/ghc/ghc/-/issues/17834.

Or maybe it's more pragmatic to just allow the code coverage to drop in those cases. The coverage isn't an end goal in itself, just an indicator, after all.

While 100% code coverage isn't a goal in itself - the problem is with everything below 100%. While you can't be sure to have tested everything you need to, when you have 100% coverage, the inverse is true: If you have any uncovered line, you can be sure that you are missing a test. Well, that is if HPC was smart enough to deal with all language extensions. That means right now, you'd have to look at every uncovered line and decide whether this is ok to be uncovered, either because it's unsupported by HPC or because we are deliberately adding that piece of code without coverage. This adds quite some mental overhead, if you want to do stuff right.

This is somewhat easier by only looking at code coverage of changed lines in the GitHub MR - but still. You'll hit that very often. It also just distracts from those tests that matter, because you are forced to look at very basic lines without much logic or complexity.

If coverage is 100% you can be sure to have covered all that simple stuff and it's much simpler to focus on those tests that actually matter.

At least that's how I feel about it.

Seems that doesn't work according to #2657 (comment).

The other option is to just add the Show to the types directly.

Yeah, that separate file seems hard to do.

How about adding Show to every type - and then just adding a simple unit test shows each type once?

But yeah, maybe that's only useful when this is actually the last thing separating us from 100%. While HPC still doesn't support record syntax, it doesn't really make that much of a difference, I guess.

[aljungberg]

If coverage is 100% you can be sure to have covered all that simple stuff and it's much simpler to focus on those tests that actually matter.

Yeah, I totally get what you're saying. With the current limitations, reaching 100% coverage is sysiphean though, so all I'm saying is let's use it for what it can be used for now. We can use the percentage as a relative indicator to confirm PRs are generally increasing or maintaining coverage. RecordWildCards may not be the only issue; in one of my recent PRs Ord was marked as uncovered even that the type was used in an ordered set.

How about adding Show to every type - and then just adding a simple unit test shows each type once?

No objections to that.

[steve-chavez]

This continues to be painful everytime I need it. For example for debugging #2846.

@wolfgangwalther Would you be opposed to me adding the Show instances and just letting coverage drop? I assume it wouldn't drop too much and we could leave it as TODO.

Additionally seems #1713 is another way to fix these drops?

Opened #2847

[wolfgangwalther]

@wolfgangwalther Would you be opposed to me adding the Show instances and just letting coverage drop? I assume it wouldn't drop too much and we could leave it as TODO.

Sure, go ahead.