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

Fix fnullable #9

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@k-bx

k-bx commented Nov 4, 2018

Fixes #8

I couldn't figure out how to run tests, so I didn't make one.

➜  json-helpers git:(master) elm-test               
It looks like you're running elm-test from within your tests directory.

Please run elm-test from your project's root directory, where its elm-package.json lives!

Help appreciated.

@k-bx

This comment has been minimized.

k-bx commented Nov 4, 2018

One thing to note is, I'm not sure if we'd like the code to still work when the key is absent or not, but I couldn't make it work. This doesn't work for example:

> Json.Decode.decodeString (Json.Decode.nullable (Json.Decode.field "foo" (Json.Decode.nullable Json.Decode.int))) "{}"            
Err (OneOf [Failure ("Expecting null") <internals>,Failure ("Expecting an OBJECT with a field named `foo`") <internals>])
    : Result Json.Decode.Error (Maybe (Maybe Int))
@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

(Looking at it, but do not worry about the tests, this was an 0.18 thing, and it was a hack anyway, there is no proper testing in Elm :( )

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

I think you can make it work by using oneOf, something like:

oneOf [ (the thing I had previously), (the thing you just wrote) ]

Does that work for you?

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

However I do not think it is a good idea, as you can't choose the behavior anymore. You can however get to what you wanted by using a standard field and prefixing the decoder by nullable, can't you?

@k-bx

This comment has been minimized.

k-bx commented Nov 5, 2018

@bartavelle I get what I want by the change in this PR, it would make the current code generated by elm-bridge work, and fnullable would have a behavior correctly working on fields with a null value. Do you see any reasons not to merge at the moment?

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

fnullable decodes missing keys as Nothing. AFAIU, your change decodes keys with null fields as Nothing. You could get the same result by using something like field "foo" (nullable decoder), or something like that, I do not have the exact Elm wording in mind :/

@k-bx

This comment has been minimized.

k-bx commented Nov 5, 2018

fnullable decodes missing keys as Nothing

Sorry, I think I don't fully understand what you mean by this claim. Do you mean:

  1. that fnullable in this package is intended to only handle the "missing field" scneario, and we should fix it plus the elm-bridge to handle the null-value case
  2. that fnullable works correctly as of right now (which I don't find to be the case) and we should only change elm-bridge?

Thanks

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

I think that both positions have merit. There is definitely a bug in Elm-bridge though! How did you generate the code? How was the missing-field thingie set in the option record?

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 5, 2018

My position would be that the case where the content of a field can be null is no different than in any other places where you can have null, and should not be handled with a specific combinator.

@k-bx

This comment has been minimized.

k-bx commented Nov 5, 2018

I generate my code with something like this:

data Foo 
  = Foo
  { fooFieldOne :: Maybe Int
  , fooFieldTwo :: String }
  deriving (Generic)

jsonOpts :: Int -> J.Options
jsonOpts n = J.defaultOptions {J.fieldLabelModifier = J.camelTo2 '_' . drop n}

deriveBoth (jsonOpts 3) ''Foo

I could definitely just set options to not render null fields, but I think this should also be fixed in elm-bridge to not confuse other users, plus I personally prefer to render fields with a null value (to have a fixed schema).

Not sure when I'll have time to make a PR to elm-bridge unfortunatley.

@k-bx

This comment has been minimized.

k-bx commented Nov 5, 2018

Forgot to add, it's lts-12.10, so it's aeson-1.3.1.1

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 6, 2018

I fixed json-helpers documentation, and I will look into the elm-bridge bug.

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 6, 2018

I incorporated your suggestion into a patch that handles both use cases before working on elm-bridge. Does that currently work for you?

@k-bx

This comment has been minimized.

k-bx commented Nov 6, 2018

I've seen the change, it looks good, no need to fix elm-bridge with it, I believe.

New version release would be appreciated. Thank you!

@k-bx k-bx closed this Nov 6, 2018

@bartavelle

This comment has been minimized.

Owner

bartavelle commented Nov 6, 2018

done! thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment