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.
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
logic hierarchy #421
base: main
Are you sure you want to change the base?
logic hierarchy #421
Changes from 2 commits
340ba59
0a884fd
acc0d5f
83f8d23
a04e9a0
5fa59a5
6e4576c
e069604
a644bf5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think some documentation describing the laws of the class could be useful here
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.
The
Conditional
(as well asEq
) instance forVector
s is derived via theGenerically1
newtype. This is because the instance definition works for anyRepresentable
(andFoldable
) over aConditional
(andEq
). But the choice of theGenerically1
newtype is arbitrary. Similarly the choice of theIdentity
newtype for derivingConditional
(andEq
) instances from their Prelude variants is arbitrary.Are there better newtypes to use for this purpose? Maybe new
Representably
andPrelusively
newtypes are warranted?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.
I think
Generically1
is fine hereThere 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.
Maybe it's better to use something else instead of
Identity
for Haskell types though...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.
Suggestion: move deriving stuff to corresponding
.Generic
/.Deriving
modules (e.g.ZkFold.Base.Data.Eq.Generic
orZkFold.Base.Data.Eq.Deriving
) to make modules shorter and more readable. I guess instances forK1
would still need to be defined in the main module, but this still would look cleaner and understandable (because it would be near instances of the main class for tuples which need it)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.
So 5 modules instead of 1? 🙃 I don't understand how this would work and not require mutually importing modules. I split it into 3 for now.
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.
ok, maybe at least visually separate the part on Generic deriving from the rest by a comment