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

Implements support for algebraic sum types #34

Open
wants to merge 13 commits into
base: devel
Choose a base branch
from

Conversation

FPtje
Copy link

@FPtje FPtje commented Feb 26, 2017

Please review before merging. Some things were done quickly, and I haven't tested aeson compatibility yet, which I still plan to do.

[update] Aeson compatibility has now been tested. See next post.

See #6

Things done

  • Added tests for encoders and decoders of Timing and Position
  • Implemented encoders and decoders for data types with multiple constructors. It was designed to be compatible with the whims of aeson, though this hasn't been tested just yet.
  • Added a new test type: Monstrosity, which revealed some bugs in the original implementation (and in my implementation of encoders and decoders)
  • Fixed those bugs

Some background

As you mentioned in this issue comment, Aeson is very specific about the json structure it generates. Differences depend on the following factors:

  • Whether one or more constructors are a record type (in Haskell)
  • Whether there are multiple constructors
  • Whether there are constructors that have arguments

Really annoying, but the code in this PR should be compatible with it, though I've yet to test that. I do know, however, that the generated code compiles with Elm 0.18 and is correctly typed. That I did test.

@FPtje
Copy link
Author

FPtje commented Mar 5, 2017

Alright, I've made a pretty cool Elm <-> ghcjs Aeson conversion test in my elm-marshall-example repository. That repository contains a website running both Elm and ghcjs. When you press a button called test, Elm sends some values of Position, Timing and my Monstrosity through some port, json encoded. Ghcjs listens to the port, prints what it received, and then sends some other values back through some Elm subscription. Elm then shows them in the website.

Here's how it looks:
2017-03-05-133420_1899x1579_scrot

Notes:

  • The website is drawn by Elm. All the console prints are from ghcjs.
  • Never mind the Person stuff in the screenshot. That one is sent back and forth directly without json being involved.
  • Due to some bug in ghcjs, Elm encodes Position to a Javascript value, while encoding Timing and Monstrosity to Json strings. This is not a problem on the Elm side, which can encode to javascript values and strings either way.

@mattjbray
Copy link
Collaborator

Hi @FPtje, thanks for getting started on this.

I haven't had a chance to review the code in detail, but here are a couple of admin items that will help move this forward:

  • Please change the target branch for this PR to devel (https://help.github.com/articles/changing-the-base-branch-of-a-pull-request/), and rebase the changes onto devel. Hopefully the rebase won't be too painful...

  • The generated code should match the output of elm-format. Also, this is just personal taste but I prefer a case expression to nested if-then-else. So, for example, decodeMonstrosity becomes:

    decodeMonstrosity : Decoder Monstrosity
    decodeMonstrosity =
        field "tag" string
            |> andThen
                (\x ->
                    case x of
                        "NotSpecial" ->
                            decode NotSpecial
    
                        "OkayIGuess" ->
                            decode OkayIGuess
                                |> required "contents" decodeMonstrosity
    
                        "Ridiculous" ->
                            decode Ridiculous
                                |> required "contents" (index 0 int)
                                |> required "contents" (index 1 string)
                                |> required "contents" (index 2 (list decodeMonstrosity))
    
                        _ ->
                            fail "Constructor not matched"
                )
    

Open questions, cc @krisajenkins:

  • In this PR, decodeUseless succeeds on any input. How does Aeson encode this type? Something like {"tag": "Useless", "contents": [null]}? Should the decodeUseless then require JSON in this structure?
  • Should the decoders be more strict about the contents field for nullary constructors? For example, this PR encodes the Start constructor as {"tag": "Start", "contents": []}. On the other hand the decoder only inspects the JSON "tag" field, so would succeed on invalid input like {"tag": "Start", "contents": ["Oops"]}. Does Aeson do an explicit check that contents is an empty array for nullary constructors? Do we care?

@FPtje FPtje changed the base branch from master to devel March 11, 2017 12:07
@FPtje
Copy link
Author

FPtje commented Mar 11, 2017

The branch has been rebased on devel. I've squashed a bunch of commits to make it look a bit nicer. The generated code now matches elm-export, both the encoders and decoders.

To answer your questions:

print $ toJSON () in Haskell gives Array []. Should decodeUseless expect an (empty) array? Maybe, but what would the type of the contents of the array be? Some dummy type? Aeson is pretty strict in the structure. It decodes [] to Just (), but decodes [1,2,3] to Nothing.

That should also answer your second question: Yes, aeson does make sure contents is an empty array. Decoding will fail when it contains stuff or is omitted.

Do we care? Not sure.

@charles-cooper
Copy link

just to chime in, this looks great. i tested against a sum constructor with one and two constructor fields (so something like data Foo = Foo Int Int | Bar String) and it got decoded by a Haskell backend.

@CarstenKoenig
Copy link

works great (would be awesome if you could merge this and release another version)

one remark though: AFAIK elm-format is using two empty lines between top-level functions but that's probably a general problem with elm-export

@mebassett
Copy link

Is anyone actively shepherding this PR or this repo or has it all been orphaned ?

@FPtje
Copy link
Author

FPtje commented Nov 7, 2017

I've decided against using this in my own projects, since Miso is a really nice Haskell alternative to Elm. I haven't looked at this since.

@tlentz
Copy link

tlentz commented May 10, 2018

Hey when is this going to be merged?

@chshersh
Copy link

I also wonder, what is the status of PR and when it's going to be merged?

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.

8 participants