Skip to content
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

Refactor: Remove (mis)uses of GHC.Show instances #2016

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

monacoremo
Copy link
Member

This is an item on our refactoring roadmap #1804

Using GHC.Show to pretty-print or serialize data is an anti-pattern, show is meant to create a Haskell expression that can be parsed by read, see https://hackage.haskell.org/package/base-4.16.0.0/docs/GHC-Show.html

This PR removes all uses of GHC.Show instances in the codebase. As a side effect, this avoids several string conversions, especially when parsing preference headers, for what is likely a small performance gain.

@monacoremo monacoremo mentioned this pull request Nov 7, 2021
27 tasks
@wolfgangwalther
Copy link
Member

I think I get the reasoning for not using GHC.Show.

But are there any reasons against using custom classes for this purpose?

Just two examples from the first commit (preferences):

  • Why not one ToAppliedHeader class for both resolutionAppliedHeader and transactionAppliedHeader?
  • Why not one ToHeaderValue class for all of resolutionStr, representationStr, parametersStr, countStr, transactionStr?

I think this would make it a bit more concise and clearly show that those functions mentioned above all share a common goal / output.

@monacoremo
Copy link
Member Author

monacoremo commented Nov 7, 2021

I think this would make it a bit more concise and clearly show that those functions mentioned above all share a common goal / output.

True, that's a bit cleaner! I tried to keep it as simple as possible in general, but it makes sense to use typeclasses in this case.

Edit: see latest commit with those changes for Preferences. We should do something similar for dumpConfig, but that's for another PR!

@wolfgangwalther
Copy link
Member

Edit: see latest commit with those changes for Preferences.

I Like that!

We should do something similar for dumpConfig, but that's for another PR!

Agreed. This could be something like ToConfigValue and should include showTxEnd and the other showXXX functions we currently have. Maybe we could even extend this class to cover some of the builtin types that we use for other config options. The class could then include the quoting etc...

And then there should be a class for ToSqlFragment. I'm sure we will find "a few" other cases where using that would make things consistent.

src/PostgREST/Request/Preferences.hs Show resolved Hide resolved
src/PostgREST/Request/Preferences.hs Show resolved Hide resolved
src/PostgREST/Config.hs Outdated Show resolved Hide resolved
@monacoremo
Copy link
Member Author

Many thanks for reviewing @wolfgangwalther ! The FreeBSD build is doing weird things again due to some cache issues, but otherwise this should be good to merge

@monacoremo monacoremo merged commit 2b22f88 into PostgREST:main Nov 9, 2021
@monacoremo monacoremo deleted the killghcshow branch November 9, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants