-
Notifications
You must be signed in to change notification settings - Fork 4
Wrap Data.Aeson.Value in a newtype SafeValue #44
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
base: master
Are you sure you want to change the base?
Conversation
|
Ah, thank you for the effort. 🙏 I'll try to look at this when I have some free time (hopefully in the next month) |
Vlix
left a comment
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 SafeValue should also be exported from Data.SafeJSON. And we'll also need an instance SafeJSON SafeValue.
I don't think we'd have to hide the data constructor of SafeValue when exporting, since it's trivial for anyone to create a SafeValue from a regular Value anyway. (because of the instance SafeJSON Value)
| version = noVersion | ||
|
|
||
| -- | @since 1.1.2.0 | ||
| instance (SafeJSON (f a), SafeJSON (g a)) => SafeJSON (Sum f g a) where |
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.
Why did you comment out this instance?
| -- @since 1.0.0 | ||
| removeVersion :: Value -> Value | ||
| removeVersion = \case | ||
| -- removeVersion :: SafeValue -> Value |
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.
This commented-out section can be removed, right?
| -- | ||
| -- @since 1.0.0 | ||
| setVersion' :: forall a. SafeJSON a => Version a -> Value -> Value | ||
| setVersion' :: forall a. SafeJSON a => Version a -> SafeValue -> SafeValue |
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 designed the setVersion functions to mainly be used to set the version when the value does not have one (taking a format from a third party who doesn't need to know about the versioning, for example)
So I'd say it would be more logical for the type to be Value -> SafeValue
#20
This isn't complete but I wanted to make sure it is going in the right direction before completing it. Things to note:
safeTocreates aSafeValue. I wasn't sure ifsafeFromshould takeValueorSafeValue.SafeValueconstructor should not be exported. Just the type and theunSafeValuefield.