-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add nub constraint on Record Show instance #269
Conversation
Looks like this can't be fixed. CI fails with:
|
I think saying "this can't be fixed" is a bit premature. It looks to me like you're (correctly) asking for a Nub constraint as well as the an ShowRecordFields instance for |
Ah... That explains the type error. |
I added a second |
This one's a bit out of my wheelhouse — perhaps @hdgarrood could take another look? |
src/Data/Show.purs
Outdated
( Nub rs rs' | ||
, Nub rs' rs |
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.
Can this be asserted with Nub rs rs
?
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.
Seems reasonable to me. I think this is breaking - if you are using show
on a record whose type isn't concrete, then you might not have a Nub rs rs
instance in scope and presumably your code would break. For example, if for some reason you wrote
myShowRecord :: forall rs ls. (RL.RowToList rs ls, ShowRecordFields ls rs) => Record rs -> String
myShowRecord = show
then I think that would break after this change.
So...
|
I think removing the bugfix entry from changelog makes sense, but I'd say my previous comment is more detail than is appropriate for the changelog. |
Description of the change
Fixes #216. I'm not sure if this would be considered breaking or not.
Checklist: