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

Showf type synonym #109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Showf type synonym #109

wants to merge 4 commits into from

Conversation

benjaminselfridge
Copy link
Contributor

@benjaminselfridge benjaminselfridge commented Apr 24, 2021

This replaces the ShowF class with a type synonym:

type ShowF f = forall tp . Show (f tp)

I don't know if we want this change, but I've made the PR to have a pros/cons discussion about it.

Pros

  • Reduces boilerplate (no more instance ShowF MyType)
  • For types defined elsewhere, we need orphan instance ShowF to get a Show instance on List, Assignment, etc. This makes that no longer necessary

Cons

  • Breaking change
  • Drops support for GHC 8.4
  • Have to define instance Show (MyType a), rather than individual instance Show (MyType Foo), instance Show (MyType Bar)

@travitch
Copy link
Contributor

If we were going to do something like this, I'd really prefer to wait until we have the automation in place to test across all known affected repositories

@benjaminselfridge
Copy link
Contributor Author

@travitch That makes sense. I know fryingpan is down, but once it's back up would that be sufficient?

This may easily be an instance of "if it ain't broke, don't fix it." But the orphan instance thing has become slightly annoying for using Data.Parameterized containers on GADTs defined in other libraries.

@travitch
Copy link
Contributor

Yes, I was referring to fryingpan. I like this idea and would love to get rid of these instances, but would really like to avoid any possibility of us getting 80% of the way to done (with some PRs merged) and then finding out that something downstream cannot use this approach for some reason that nobody foresaw.

@benjaminselfridge
Copy link
Contributor Author

Great, then I'll wait until fryingpan is back up before pushing this forward. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants