Could `JPropCodec` be defined as `BasicCodec (Either JsonDecodeError) (Object Json)`?

joneshf commented 5 years ago

I haven't looked at the implementation to see if there's a reason it requires the list implementation, but they should be equivalent. The reason I ask is that Data.Codec.basicCodec could be used, which would make constructing a JPropCodec _ just a bit easier as you wouldn't have to import (or directly depend on) transformers. It's totally fine if you'd prefer not to change it; this was something that caused a little friction while I was initially learning how things work and how to define a JPropCodec _.

garyb commented 5 years ago

That definitely seems like something worth exploring, but yeah, I think the implementation might need to change a bit. It seems like Codec would need to use State rather than Writer for one thing.

I would say the types aren't quite equivalent either - JPropCodec is for a values that map to key/value pairs inside an object, and it encodes/decodes Object Json values, rather than it encoding/decoding Json values which happen to be an object with key/value pairs. That's why CA.object exists as a separate thing as it is - it deals with the object encoding/decoding, but passes off the key/value handling to the JPropCodec it's constructed with. It's also why CA.record needed it, since it constructs a JPropCodec rather than a JsonCodec for a record.

Here's the JsonCodec synonym expanded to Codec rather than BasicCodec for a comparison:

type JPropCodec a =
  Codec
    (Either JsonDecodeError)
    (FO.Object J.Json)
    (L.List (Tuple String J.Json))
    a
    a

type JsonCodec a = 
  Codec 
    (Either JsonDecodeError) 
    J.Json 
    J.Json
    a
    a

garyb commented 5 years ago

Sorry, I just re-read your proposed type for JPropCodec in the issue title and see I lectured you on a bunch of stuff you've already worked out. The answer is still "I don't know, will need to try it" though :smile:. I think it can work without the State, but we might need a newtype to give Object Json a monoid/semigroup instance that does what we want.

joneshf commented 5 years ago

Sorry, I should have been clearer what I was asking.

I assume the Monoid/Semigroup stuff is for the Writer _ instances. If so, that makes sense. It's also something that would seem fine, but make someone downstream unable to use this package as easily if they were dependent on those instances. E.g. code here might continue to compile, but there's no Semigroup instance for Json so anyone that was reliant on the Bind instance would have to do things differently. That doesn't seem worth it.

No worries, figured it was worth asking. Thanks for the insight!

garyb commented 5 years ago

I'll keep this open actually as something to think about. I was wondering if ST could be used in place of Writer somehow, as that way we could be much more efficient during encoding by mutating objects and arrays while we construct them, rather than having to build the intermediate list / repeatedly extend the Object if JPropCodec was reworked as you suggested.

joneshf commented 5 years ago

Sounds good.

I was also thinking about what I really wanted with opening this issue. I think the main thing is to not have to directly depend on purescript-transformers. I think that might be solvable in purescript-codec though. It might also be a thing that is already possible with Data.Codec.mapCodec and Data.Codec.Argonaut.record.

joneshf commented 5 years ago

I looked into this some more, and this seems to type check and give me what I wanted:

jPropCodec ::
  forall a.
  (Foreign.Object.Object Data.Argonaut.Json -> Data.Either.Either Data.Codec.Argonaut.JsonDecodeError a) ->
  (a -> Foreign.Object.Object Data.Argonaut.Json) ->
  Data.Codec.Argonaut.JPropCodec a
jPropCodec decode encode = 
  Data.Codec.GCodec
    (Control.Monad.Reader.ReaderT decode)
    (Data.Profunctor.Star.Star \x ->
      Control.Monad.Writer.writer
        (Data.Tuple.Tuple x (Foreign.Object.toUnfoldable (encode x)))
    )

Instead of talking at the transformer level, we talk at the object level. Would you be willing to accept a PR to add this value?

joneshf commented 5 years ago

Providing a value to construct a Data.Codec.Codec _ _ _ _ _ makes this easier:

codec ::
  forall a b c d f.
  (a -> f d) ->
  (c -> Tuple d b) ->
  Codec f a b c d
codec decode encode =
  Data.Codec.GCodec
    (Control.Monad.Reader.ReaderT decode)
    (Data.Profunctor.Star.Star \x -> Control.Monad.Writer.writer (encode x))

jPropCodec ::
  forall a.
  (Foreign.Object.Object Data.Argonaut.Json -> Data.Either.Either Data.Codec.Argonaut.JsonDecodeError a) ->
  (a -> Foreign.Object.Object Data.Argonaut.Json) ->
  Data.Codec.Argonaut.JPropCodec a
jPropCodec decode encode = codec decode \x ->
  Data.Tuple.Tuple x (Foreign.Object.toUnfoldable (encode x))

But I'll leave that discussion for purescript-codec.

garyb / purescript-codec-argonaut

Could `JPropCodec` be defined as `BasicCodec (Either JsonDecodeError) (Object Json)`? #24