Skip to content
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

Generic deriving #3

Merged
merged 1 commit into from
Aug 25, 2015
Merged

Generic deriving #3

merged 1 commit into from
Aug 25, 2015

Conversation

zudov
Copy link
Contributor

@zudov zudov commented Aug 24, 2015

It should be quite ready now.

@@ -37,6 +44,44 @@ import qualified Data.Map as Map
class DecodeJson a where
decodeJson :: Json -> Either String a

-- | Decode `Json` representation of a value which has a `Generic` type.
gDecodeJson :: forall a. (Generic a) => Json -> Either String a
gDecodeJson = map (fromJust <<< fromSpine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should eliminate this fromJust and instead return a Left with some appropriate error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdegoes That's right. Fixed.

@jdegoes
Copy link
Contributor

jdegoes commented Aug 25, 2015

Looks great! Three minor nitpicks:

  1. Can you squash commits?
  2. The to / from list will incur a performance overhead, if you can directly fold over the map structure (at least for serializing to JSON), that will be faster.
  3. It would be nice to bulk up the test data structure a bit to make the property tests more conclusive.

👍 Thanks for your work on this!

@zudov
Copy link
Contributor Author

zudov commented Aug 25, 2015

@jdegoes Nitpicks are welcome.

  1. I'll squash everything after we review these last bits
  2. See 60b2e29 and 57b2d72, not sure if that's the best what we can do without giving up safety but it looks much better.
  3. I can move data declarations a bit closer to the printing part, or to move printing part into a separate function and put it close to data declaration, is that what you meant?

@jdegoes
Copy link
Contributor

jdegoes commented Aug 25, 2015

(1) and (2) look fine. For (3), I meant, create a more complex data structure to exercise all the types exposed in Generic.

@zudov
Copy link
Contributor Author

zudov commented Aug 25, 2015

@jdegoes Makes sense. Fixed.

@jdegoes
Copy link
Contributor

jdegoes commented Aug 25, 2015

👍 Awesome work! Can you squash commits one more time and I'll merge in?

@zudov
Copy link
Contributor Author

zudov commented Aug 25, 2015

Upps. Broke it while rebasing, fixing now.

@zudov
Copy link
Contributor Author

zudov commented Aug 25, 2015

Ready to be merged now

jdegoes added a commit that referenced this pull request Aug 25, 2015
@jdegoes jdegoes merged commit 0f004b6 into purescript-contrib:master Aug 25, 2015
@jdegoes
Copy link
Contributor

jdegoes commented Aug 25, 2015

👍 Thanks again!

@zudov
Copy link
Contributor Author

zudov commented Aug 25, 2015

@jdegoes Thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants