-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
You probably want to use npm-check-updates and run |
@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 |
is it ok if I change the .travis.yml to something like https://github.com/justinwoo/purescript-simple-json/blob/master/.travis.yml ? |
sorry for the stupid commits/patches - sometimes I miss the most obvious things would love to hear opinions on this |
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 |
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 replace id with a hand-written id?
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 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
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.
id -> identity
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 I see - that's identity
from Control.Category
right?
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's in prelude anyway
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.
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
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's why there's typed hole search: https://github.com/paf31/24-days-of-purescript-2016/blob/master/23.markdown
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.
fair enough I guess (I'm probably not using that feature enough)
Hey @CarstenKoenig is there any progress on this or have you abandoned it? |
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. |
I updated the dependencies - hopefully it'll now run the test on travis successfully too |
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 |
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 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) |
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.
identity
?
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 yes ... it took me some time to find it and my tooling was broken ... I'll fix it
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? |
I opened a clean version of this here: #8 - hope this helps you |
Regarding documentation - I meant some light description of what the classes and their variables mean. For example, the meaning of 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 Closing this since #8 is merged; let me know if there's stuff here you need me to re-open. |
no it's all right - everything here was in #8 - thanks again for merging |
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 usedRowToList
to get records working again using techniques borrowed/inspired from here: https://github.com/justinwoo/purescript-simple-jsonFor 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
andargonaut-codecs
) - so that probably have to go too.