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

TaggedObject encoding for Nullary constructor does not have "contents" field #307

Merged
merged 2 commits into from May 6, 2016

Conversation

devwout
Copy link
Contributor

@devwout devwout commented Oct 20, 2015

As requested in #300 , implemented this for both GHC Generics and TemplateHaskell .

@@ -201,7 +201,6 @@ instance ( IsRecord a isRecord
(builder tagFieldName <>
B.char7 ':' <>
builder (constructorTagModifier opts (conName (undefined :: t c a p)))) <>
B.char7 ',' <>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit mysterious. Can you explain what's going on here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I see what's going on: you moved the comma insertion elsewhere. Is that correct?

(Ideally, the movement of comma insertion would have been a separate standalone commit, and it would thus have been obvious what was going on.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, I moved the comma insertion. Otherwise, the encoding for nullary constructor would be {"tag": "X",} (trailing comma). I agree a separate commit would have made this more clear, I'll keep it in mind.

@devwout
Copy link
Contributor Author

devwout commented Dec 1, 2015

Hi @basvandijk

I updated my pull request based on your suggestions and it still works as expected. Explicitly specifying the case for nullary constructors is indeed simpler.

If you prefer me to amend these two commits to the previous one, tell me. We are all busy, I understand this took a while.

@bos
Copy link
Collaborator

bos commented Jan 19, 2016

@devwout if you could please squash and rebase your commits, I'd appreciate it. Thank you.

@devwout devwout force-pushed the nullary_constructor_encoding branch from 9eec9a9 to c072f7a Compare January 20, 2016 22:41
@devwout
Copy link
Contributor Author

devwout commented Jan 20, 2016

Hi @bos

The pull request is rebased and squashed into two commits. I think it is best to commit the tests for existing functionality separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants