search

purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
97 stars 80 forks source link

Remove Show instances from registry types #538

Closed thomashoneyman closed 2 years ago

thomashoneyman commented 2 years ago

We should only be using Show for our tests, and since spec no longer requires a Show instance we no longer need to use these in our code. It's quite easy to slip up and use show for convenience, such as in an error message, only to inadvertently leak the internal structure of the type instead of pretty-printing, or the opposite: to make the show instance pretty-print, when it is supposed to show the structure of the type for debugging. As a last argument against Show, all these instances and their associated Generic instances add to quite a bit of code bloat in the generated code.

As part of the general registry cleanup, this PR removes our use of Show throughout the codebase and updates the tests to accommodate the change.

thomashoneyman commented 2 years ago

(Ideally we merge #537 before this one, as they have some duplicate work.)

f-f commented 2 years ago

I wouldn't want to ship this without taking care of the issue "how do we log these?". The Show instance is used today in Spago and it is very convenient for debug logging. I understand and agree with the arguments and their principle, but practicality is also very important.

How about adding Debug instances? Until the compiler has builting support these would still require the Generic instance though, so I am not sure what this buys us.

f-f commented 2 years ago

I see you added an unsafeStringify, is that the recommended method?

thomashoneyman commented 2 years ago

@f-f Are you wanting to show the internal structure of things, like the data constructor? In our existing logs we’re showing the printed versions of things (the data the user would actually be sending to us). So for that we should use the various print functions or encode as JSON and print (aka “stringifyJson” or “printJson”).

But if you want to show the internal structure then we do need something like Show, except we should ban its use in normal logs and have those instances actually represent the type’s internal structure.

I was assuming we would go with the first option (print / encode json) so it’s more readable — for example, it’s terrible trying to look at the Show output of a Map, even for debugging.

thomashoneyman commented 2 years ago

unsafeStringify is only for tests where we don’t want to have to pass around codecs — shouldn’t be used in the actual codebase

f-f commented 2 years ago

Makes sense - what I was trying to say really was that we should make sure that we have some kind of print function for every datatype where we remove the Show instance. This PR is fine in any case because it's concerning only the app, but we should keep this in mind once we start moving things to the lib folder.