Skip to content

WIP: records with RowToList #6

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

Closed

Conversation

CarstenKoenig
Copy link
Contributor

Hi,

this is still work in progress - but is this something you would appreciate?

Basically this should be the first step to move this forward to PureScript 0.12.0

I removed the purescript-generics parts which are no longer suported AFAIK and used RowToList to get records working again using techniques borrowed/inspired from here: https://github.com/justinwoo/purescript-simple-json


For the tests I just upgraded to Effect and all are green and look fine to me (but please take a look)

What is probably not ready to be merged is the version bounds for the bower.json (I might have messed up some version numbers and maybe I missed one all together)

I developed/tested this using psc-package with my own package-set (in which I just included argonaut-core and argonaut-codecs) - so that probably have to go too.

@justinwoo
Copy link
Contributor

You probably want to use npm-check-updates and run ncu -ua at least to upgrade the tooling. The others you might point to your own #compiler/0.12 branches or whatnot

@CarstenKoenig
Copy link
Contributor Author

@justinwoo you mean the Travis CI tooling? I don't have much experience with this but I'll try

to be honest: I expected something like this - that's why I WIPed

@CarstenKoenig
Copy link
Contributor Author

is it ok if I change the .travis.yml to something like https://github.com/justinwoo/purescript-simple-json/blob/master/.travis.yml ?

@CarstenKoenig
Copy link
Contributor Author

sorry for the stupid commits/patches - sometimes I miss the most obvious things

would love to hear opinions on this

@CarstenKoenig
Copy link
Contributor Author

this is supposed to work towards "fixing" #7

test/Main.purs Outdated
@@ -43,23 +43,23 @@ derive instance genericLiteralStringExample :: Generic LiteralStringExample _
instance showLiteralStringExample :: Show LiteralStringExample where
show a = genericShow a
instance encodeJsonLiteralStringExample :: EncodeJson LiteralStringExample where
encodeJson a = encodeLiteralSumWithTransform id a
encodeJson a = encodeLiteralSumWithTransform (\x->x) a
Copy link

Choose a reason for hiding this comment

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

Why replace id with a hand-written id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes - thanks for reminding me, I wanted to check that out

for some reason id was not in scope anymore and instead of trying to add yet another yak on my shaving stack I opted to just put it in myself

I'll try to check this out tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

id -> identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see - that's identity from Control.Category right?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's in prelude anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah ide-server figured that out for me ;)

I understand the reasoning but it's really hard to find using pursuit when you look for the usual a -> a

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough I guess (I'm probably not using that feature enough)

@fsoikin
Copy link

fsoikin commented Jun 25, 2018

Hey @CarstenKoenig is there any progress on this or have you abandoned it?

@CarstenKoenig
Copy link
Contributor Author

Hi @fsoikin - not at all - I guess I should have a look and see if the dependencies are all updated by now (they probably are) - the code itself should work and I would love some comments on it.

I'm at a conference right now but I'll try to give a in the next few days.

@CarstenKoenig
Copy link
Contributor Author

I updated the dependencies - hopefully it'll now run the test on travis successfully too

@CarstenKoenig
Copy link
Contributor Author

ok seems fine - what do you think?

If it's ok with you I'll gonna rebase it, close this one and submit a new/clean PR

Copy link

@fsoikin fsoikin left a comment

Choose a reason for hiding this comment

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

This looks ok to me.

I wouldn't mind some comments on classes and instances, but overall fine.

@@ -95,7 +125,7 @@ mFail msg = maybe (Left msg) Right

-- | A function for decoding `Generic` sum types using string literal representations
decodeLiteralSum :: forall a r. Rep.Generic a r => DecodeLiteral r => Json -> Either String a
decodeLiteralSum = decodeLiteralSumWithTransform id
decodeLiteralSum = decodeLiteralSumWithTransform (\a -> a)
Copy link

Choose a reason for hiding this comment

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

identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes ... it took me some time to find it and my tooling was broken ... I'll fix it

@CarstenKoenig
Copy link
Contributor Author

concerning the documentation: I'm not sure what style you want here - especially for the instances - do you want some kind of high-level explanation on how the parts fit together?

@CarstenKoenig
Copy link
Contributor Author

I opened a clean version of this here: #8 - hope this helps you

@fsoikin
Copy link

fsoikin commented Jun 26, 2018

Regarding documentation - I meant some light description of what the classes and their variables mean. For example, the meaning of from and to in DecodeRepRowList. I can see what they do, but I'm not quite sure what meaning you ascribe to them, and why they are called this way. That sort of thing.

This code is dense and abstract enough that it takes a significant effort to get through it, and some explanation of how to read it would be helpful.

Not a hard demand by any means. I'm not even a stakeholder in this repo, just a consumer.

@CarstenKoenig
Copy link
Contributor Author

@fsoikin like this: bf231ab?

(sorry I'm bad at this stuff - it's probably to much and at the same time not enough - if you can give me some feedback I'll try to improve and add the same to the encoding side as well)

@thomashoneyman
Copy link
Contributor

@CarstenKoenig Closing this since #8 is merged; let me know if there's stuff here you need me to re-open.

@CarstenKoenig
Copy link
Contributor Author

no it's all right - everything here was in #8 - thanks again for merging

@CarstenKoenig CarstenKoenig deleted the RowToList branch August 7, 2018 20:07
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.

5 participants