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

Use handwritten JSON instances for Type/Kind #3496

Merged

Conversation

natefaubion
Copy link
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
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
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
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
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.

Copy link
Contributor

@hdgarrood hdgarrood left a comment

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
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.

None yet

2 participants