Add context parameter to *Table types#101
Merged
shane-circuithub merged 2 commits intomasterfrom Jul 5, 2021
Merged
Conversation
This allows to get rid of all the `Tag` nonsense and to further simplify the `Nullifiable` nonsense. Arguably we should have done it like this in the first place, as it brings a lot of nonsense that was hiding inside `Tag` out into the open. It was nice to not have an extra redundant-seeming type parameter (because the `a` already has a `Context`), but it isn't actually redundant. The proposed solution to the `Projection` problem will definitely need this extra `context` parameter and there are no tricks with `Tag` we can do to get around that.
ocharles
reviewed
Jul 3, 2021
| {-# language MultiParamTypeClasses #-} | ||
| {-# language StandaloneKindSignatures #-} | ||
| {-# language TypeFamilies #-} | ||
| {-# language TypeFamilyDependencies #-} |
Contributor
There was a problem hiding this comment.
I think you either forgot to use this - line 41 has no dependency
ocharles
approved these changes
Jul 3, 2021
Contributor
ocharles
left a comment
There was a problem hiding this comment.
It was great we got so far without this, but as we've both found now - there's a ton of value to be derived from having this in the type. So while it's a bit of a shame, I'm on board with this.
Contributor
|
I dunno if you wanna confirm this actually unblocks the Projection stuff first, or maybe you've already checked that. Just thinking of avoiding an incomplete solution, but of course we could always revert if needed |
In this case, the extra `Context` parameter is actually 100% redundant as things stand, but it opens up the possibility of a `Prelude.Functor` instance for for `ListTable` further down the line, and at least remains "consistent" with the other `*Table` types.
e23d6e3 to
51fe694
Compare
ocharles
added a commit
that referenced
this pull request
Jul 6, 2021
ocharles
added a commit
that referenced
this pull request
Jul 6, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows to get rid of all the
Tagnonsense and to further simplify theNullifiablenonsense.Arguably we should have done it like this in the first place, as it brings a lot of nonsense that was hiding inside
Tagout into the open.It was nice to not have an extra redundant-seeming type parameter (because the
aalready has aContext), but it isn't actually redundant. The proposed solution to theProjectionproblem will need this extracontextparameter and there are no tricks withTagwe can do to get around that.