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

updated to PureScript 0.12 #8

Merged
merged 5 commits into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@CarstenKoenig
Copy link
Contributor

CarstenKoenig commented Jun 26, 2018

What does this pull request do?

  • updated dependencies
  • replaced removed Generic.Rep cases/fields with a RowToList approach

Where should the reviewer start?

the relevant changes should be in Data.Argonaut.Decode.Generic.Rep and Data.Argonaut.Encode.Generic.Rep

How should this be manually tested?

npm run test

or

pulp test

should do

Other Notes:

this is a cleaned up version from this PR: #6 (review)

updated to PureScript 0.12
- updated dependencies
- replaced removed `Generic.Rep` cases/fields with a `RowToList` approach
-- | JSON in to the resulting record value
-- |
-- | `from` and `to` indicates the the step the `Builder` will take from a `Record from`
-- | to a `Record to` so for a given `RowList` `from` will be the empty record

This comment has been minimized.

@fsoikin

fsoikin Jun 27, 2018

If from is always an empty record, then why is it variable here?

This comment has been minimized.

@fsoikin

fsoikin Jun 27, 2018

I think there is a misunderstanding.

I do understand how this works. I've already spent time to understand it, and also I happen to have written similar things multiple times before.

The comment is not for me, but for future audience. Maintainability, you know.

This comment has been minimized.

@CarstenKoenig

CarstenKoenig Jun 27, 2018

Contributor

oh shit sorry - maybe you can help me out with the wording here then

This comment has been minimized.

@CarstenKoenig

CarstenKoenig Jun 27, 2018

Contributor

I'm really only trying to help - of course you know this ...

anyway can you have a look at it now? I hope the confusing pieces are gone and it's more obvious why to use this(?)

This comment has been minimized.

@fsoikin

fsoikin Jun 27, 2018

Don't worry too much about it, I only suggested.
I think this comment is great.

-- | JSON in to the resulting record value
-- |
-- | `from` and `to` indicates the the step the `Builder` will take from a `Record from`
-- | to a `Record to` so for a given `RowList` `from` will be the empty record

This comment has been minimized.

@fsoikin

fsoikin Jun 27, 2018

Don't worry too much about it, I only suggested.
I think this comment is great.

@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Aug 7, 2018

@fsoikin Since you've got an approved review in, I'll merge this tomorrow unless you have any objections. Thanks @CarstenKoenig for your work!

@CarstenKoenig

This comment has been minimized.

Copy link
Contributor

CarstenKoenig commented Aug 7, 2018

thank you!

@fsoikin

This comment has been minimized.

Copy link

fsoikin commented Aug 7, 2018

No objections, please merge. Thank you! :-)

@thomashoneyman thomashoneyman merged commit 96ec4b0 into purescript-contrib:master Aug 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thomashoneyman

This comment has been minimized.

Copy link
Member

thomashoneyman commented Aug 7, 2018

👍

@thomashoneyman thomashoneyman referenced this pull request Aug 7, 2018

Closed

PureScript 0.12 #7

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