-
Notifications
You must be signed in to change notification settings - Fork 90
Complete daniel-chambers' PR adding Show instances to generics rep types #250
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Helps one see the generic representation for a given value
test/Data/Generic/Rep.purs
Outdated
assert "Checking show for generic types" $ | ||
show (G.from $ cons 1 Nil) == "(Inr (Constructor \"Cons\" (Argument { head: 1, tail: Nil })))" | ||
|
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.
Could we also test the show output for G.from Nil
to cover NoArguments
and the Inl
constructor, as well as a constructor with multiple arguments to cover Product
?
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.
Yup. Fixed.
src/Data/Generic/Rep.purs
Outdated
-- | A representation for constructors which includes the data constructor name | ||
-- | as a type-level string. | ||
newtype Constructor (name :: Symbol) a = Constructor a | ||
|
||
instance showConstructor :: (IsSymbol name, Show a) => Show (Constructor name a) where | ||
show (Constructor a) = "(Constructor " <> show (reflectSymbol (Proxy :: Proxy name)) <> " " <> show a <> ")" |
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 I request that we put a @
symbol before the constructor name? It wouldn’t be valid syntax (we don’t have explicit type applications) but it does at least clarify that the string isn’t an actual constructor argument for Constructor.
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.
Meaning this? Yeah, I think that makes sense.
- "(Constructor " <> show (reflectSymbol ...
+ "(Constructor @" <> show (reflectSymbol ...
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.
Yeah, exactly that.
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.
Fixed.
…pes (purescript#250) * Add Show instance to generic rep types (complete daniel-chambers PR) Helps one see the generic representation for a given value * Add `@` in front of constructor name in Show instance for generic type * Add tests for Inl, NoArguments, and Product
Completes the PR he started in purescript-deprecated/purescript-generics-rep#11