Skip to content

Use handwritten JSON instances for Type/Kind - #3496

Merged
natefaubion merged 1 commit into
purescript:masterfrom
natefaubion:types-kinds-json-instances
Dec 23, 2018
Merged

Use handwritten JSON instances for Type/Kind#3496
natefaubion merged 1 commit into
purescript:masterfrom
natefaubion:types-kinds-json-instances

Conversation

@natefaubion

Copy link
Copy Markdown
Contributor

Fixes #3494

I think this should be compatible, though I'm not super familiar with Aeson's encoding. I don't think there are any tests for these, but I can probably add some.

toJSON = kindToJSON toJSON

kindFromJSON :: forall a. Parser a -> (Value -> Parser a) -> Value -> Parser (Kind a)
kindFromJSON defaultAnn annFromJSON = A.withObject "Kind" $ \o -> do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the old kind parsing from 0.10. I don't think it's necessary to preserve these anymore.

KUnknown <$> annFromJSON' <*> key "contents" (nth 0 asIntegral)
"Star" ->
kindType <$> defaultAnn
KUnknown a <$> contents

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure this parser was broken. It appears to parse with nth 0, but it was not encoded with a wrapper list.

@natefaubion

Copy link
Copy Markdown
Contributor Author

Are these codecs exercised by the Docs tests? I guess the KUnknown case didn't arise because they only exist during checking.

@hdgarrood

Copy link
Copy Markdown
Contributor

They are only partially exercised by the Docs tests; we only test that the parser is compatible with the json produced by the current version. I've been meaning to add some tests to ensure that we don't accidentally break compatibility, because we don't have those right now.

@hdgarrood hdgarrood left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! I think once this is merged, it'll probably be a good moment to finally get around to putting together some tests which check we can still parse json generated by older compilers. I'm happy to take that on (unless you've already started looking at it)?

@natefaubion
natefaubion merged commit cdca9cc into purescript:master Dec 23, 2018
@natefaubion
natefaubion deleted the types-kinds-json-instances branch December 23, 2018 19:14
@natefaubion

Copy link
Copy Markdown
Contributor Author

I have not looked into it. Be my guest 😁

@garyb garyb mentioned this pull request Jan 12, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants