-
Notifications
You must be signed in to change notification settings - Fork 22
add encoding of string literals to sum types of unary constructors #3
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
Conversation
pure $ Rep.Constructor (Rep.NoArguments) | ||
|
||
type FailMessage = """ | ||
`decodeLiteralSum` can't be used without sum types, where all of the constructors aren't unary. This is because a string literal cannot be encoded into a product type. |
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.
Hello multi-negative! 😜
How about something like:
type FailMessage = """`decodeLiteralSum` can only be used with sum types where all of the constructors are unary. A string literal cannot sensibly encode a product type."""
for these messages?
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.
Ah, sure. Probably best to get rid of the white space too since the compiler doesn't strip them
-- | A function for decoding `Generic` sum types using string literal representations | ||
-- | Takes a function for transforming the tag name in encoding | ||
decodeLiteralSum :: forall a r. Rep.Generic a r => DecodeLiteral r => (String -> String) -> Json -> Either String a | ||
decodeLiteralSum tagNameTransform = map Rep.to <<< decodeLiteral tagNameTransform |
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.
Do you think it makes sense to provide a transform? I'd keep it simple and drop that. If you're using generics you already can't really control the format, so providing something to customise it for this particular case seems a little odd.
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.
It gets really sad if you can't convert things like CONST_STRING
to ConstString
and MYSTRING
to MyString
though, ending up with either the ugliest constructors (if not illegal) or having to provide extra things just to work with some serialized data
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 guess that's what I'm not understanding perhaps... why would you need to do that? If you're using generics it seems like it's something you would have serialized in the first place, so the specific encoding should be the same anyway?
It just seems a bit odd to me, as if you're using this argument you'd probably need an iso anyway, so you could just combine the encode/decode with a map
as appropriate.
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.
How about we provide the current functions as decodeLiteralSum'
or decodeLiteralSumWithTransform
or something, and then a version with id
applied as the normally named version?
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.
That sounds fine to me, I'll try to get it pushed up
d4815d0
to
4b22010
Compare
Pushed up the changes. Also realized this should say "nullary constructor", oops. |
4b22010
to
8cd2688
Compare
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.
Almost there!
Left $ decodingErr "string literal " <> tag <> " had an incorrect value." | ||
pure $ Rep.Constructor (Rep.NoArguments) | ||
|
||
type FailMessage = """`decodeLiteralSum` can only be used with sum types, where all of the constructors aren't nullary. This is because a string literal cannot be encoded into a product type.""" |
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.
where all of the constructors aren't nullary
Should be
where all of the constructors are nullary
😄
noooo
…On Wed, Jul 19, 2017 at 10:04 PM, Gary Burgess ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Almost there!
------------------------------
In src/Data/Argonaut/Decode/Generic/Rep.purs
<#3 (comment)>
:
> +class DecodeLiteral r where
+ decodeLiteral :: (String -> String) -> Json -> Either String r
+
+instance decodeLiteralSumInst :: (DecodeLiteral a, DecodeLiteral b) => DecodeLiteral (Rep.Sum a b) where
+ decodeLiteral tagNameTransform j = Rep.Inl <$> decodeLiteral tagNameTransform j <|> Rep.Inr <$> decodeLiteral tagNameTransform j
+
+instance decodeLiteralConstructor :: (IsSymbol name) => DecodeLiteral (Rep.Constructor name (Rep.NoArguments)) where
+ decodeLiteral tagNameTransform j = do
+ let name = reflectSymbol (SProxy :: SProxy name)
+ let decodingErr msg = "When decoding a " <> name <> ": " <> msg
+ tag <- mFail (decodingErr "could not read string for constructor") (toString j)
+ when (tag /= tagNameTransform name) $
+ Left $ decodingErr "string literal " <> tag <> " had an incorrect value."
+ pure $ Rep.Constructor (Rep.NoArguments)
+
+type FailMessage = """`decodeLiteralSum` can only be used with sum types, where all of the constructors aren't nullary. This is because a string literal cannot be encoded into a product type."""
where all of the constructors aren't nullary
Should be
where all of the constructors *are* nullary
😄
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACSS_jNOXgiUQO4PutB9fgcXx7F6WH3Rks5sPlNUgaJpZM4OY-GO>
.
|
8cd2688
to
926213b
Compare
Okay, fixed that nonsense. I'll just leave the single thing for now, and then maybe in the future we'll be able to have a catch-all instance for all the unsupported ones and make a more generic error. |
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.
Thanks!
Adds encoding of enum-style sum types and provides a function argument for how tag names are to be transformed.