decoder implementation that is not dependent on class -> move implementation to Implementation module #74

srghma-old · 2020-05-02T19:34:43Z

What does this pull request do?

I need these functions that are not dependent on class in one of my projects

I cannot use https://github.com/garyb/purescript-codec-argonaut/ because I need only decoder part and encoder part doesnt allow to make my datatype a functor

includes:
#73
#72

srghma-old · 2020-05-03T12:16:11Z

why this change:

In one project I needed to pass custom decoder function to decodeJson, but passing them like

(x :: Array Json) <- decodeJson
traverse childDecoder x

breaks error messages

but the

decodeArray childDecoder

doesnt

thomashoneyman · 2020-05-08T17:13:35Z

You can also write functions of the type Json -> Either String MyType without using the class-based decodeJson function, if you want.

I agree there could be value in implementing a Argonaut.Decode.Decoders module alongside Argonaut.Decode.Class which implements decoding functions of the type Json -> Either String SomeType, and which is imported into Argonaut.Decode.Class as the implementation for each of the classes. We'd do the same thing for encoding, then.

But we should do that without introducing typed errors at the same time. Type signatures for the class-based functions should be unchanged.

Other maintainers (@garyb) may have a different opinion on whether this would be a welcome change for the library, but I am open to it.

thomashoneyman · 2020-05-08T17:17:51Z

As an example from the README on the writing functions bit:

https://github.com/purescript-contrib/purescript-argonaut-codecs#decoding-with-more-arguments-than-json

srghma-old · 2020-05-08T18:34:21Z

You can also write functions of the type Json -> Either String MyType without using the class-based decodeJson function, if you want.

yes, I know, as I told I needed to compose custom decode functions with derived decode functions, but

(x :: Array Json) <- decodeJson
traverse childDecoder x

was breaking error messages (e.g. when childDecoder throws an error, the At INDEX wasn't prepended to error message from childDecoder)

that's why I did this pr

But we should do that without introducing typed errors at the same time

yes, I can remove custom error messages from this pr, until decision is not done, and do the same for Encoders

garyb · 2020-05-09T16:23:42Z

I don't really have much of an opinion about future directions for the library, as I don't use it at all now. 😉

If I'm writing a pure decoder rather than using my Codec thing then I write it using the functions in argonaut-core: I only used this library for serialization originally, as it's mostly valuable when you're in control of the full format, as if you're just parsing someone else's format it rarely works out that the provided instances actually do what you want - and those serialization scenarios are where I use Codec now.

I think it's a shame simple-json uses Foreign rather than the core argonaut Json type, as providing "automatic" handling for only the prim/JSON types would have more value to me. I want to be forced to explicitly handle anything more complex than that.

But that's just, like, my opinion, man. I know it's far from universally shared.

srghma-old · 2020-05-15T13:53:11Z

seems like @garyb is not against :-)

As a sidenote: I agree that Codec decoders/encoders are better than class decoders

I tried to store JsonCodec here https://github.com/srghma/purescript-fernet/blob/ba2033dac18308ec57129768fa8579c60a16c153/src/Graphql/SelectionSet.purs#L169

instead of Decoder. But I need my SelectionSet to be functor, and encoder part of JsonCodec wasn't allowing me to do that

yes, I could store there not a JsonCodec, but only decoder part of JsonCodec extracted using decoder someJsonCodec, but I have decided that decoder from this repo will be a more lightweight solution / will result in less code in browser

maybe it makes sense to do the same for https://github.com/garyb/purescript-codec-argonaut as in this pr, i.e. extract decoder implementations to the one module, extract encoders to the other module and join them again together in JsonCodec? just a thought

thomashoneyman · 2020-05-15T14:41:14Z

@srghma I’m also primarily a user of codec-argonaut these days :) but I still want to be careful with breaking changes here that will require users to update their code.

For that reason I’m happy to move decoders / encoders to their own modules that get imported to the class. And I’m not completely against changing the error type after that (but I’d like to propose it on Discourse, perhaps, so that more active users of the library have a chance to weigh in).

thomashoneyman · 2020-05-15T14:44:01Z

Just to clarify, that’s also what I meant earlier:

But we should do that without introducing typed errors at the same time. Type signatures for the class-based functions should be unchanged.

Can you either update this PR or make a new one that only moves the decoder and encoder implementations, but doesn’t also make the typed errors change?

srghma-old · 2020-05-15T14:44:37Z

And I’m not completely against changing the error type after that

very good 👍

there are anyway will be breaking changes to the libraries with new purescript PolyKinds)

srghma-old · 2020-05-15T14:46:18Z

Just to clarify, that’s also what I meant earlier:

But we should do that without introducing typed errors at the same time. Type signatures for the class-based functions should be unchanged.

Can you either update this PR or make a new one that only moves the decoder and encoder implementations, but doesn’t also make the typed errors change?

yes, I understood from the first time, will do when I have time)

…mplementation to Implementation module

srghma-old · 2020-05-22T17:48:40Z

@thomashoneyman done

thomashoneyman · 2020-05-26T17:31:00Z

src/Data/Argonaut/Decode/Implementation.purs

+    <<< (traverse decoder <=< (lmap ("JSON Array" <> _) <<< rmap (\x -> NEA.cons' x.head x.tail) <<< note " is empty" <<< Arr.uncons) <=< decodeJArray)
+
+decodeNonEmpty_List :: ∀ a . Decoder a -> Decoder (NonEmpty List a)
+decodeNonEmpty_List decoder =


Is it possible to use a single decodeNonEmpty which can be used with NonEmpty List as well as NonEmpty Array, in which case we can support a decodeNonEmpty function -- thus removing decodeNonEmpty_Array and decodeNonEmpty_List from this module?

We'd then use it to implement both the NonEmpty List and NonEmpty Array instances in the Class module.

Make array a functor? But they also have different error message

thomashoneyman · 2020-05-26T17:33:54Z

src/Data/Argonaut/Decode/Implementation.purs

+import Data.Tuple (Tuple(..))
+import Foreign.Object as FO
+
+type Decoder a = Json -> Either String a


I'm on the fence about introducing a type synonym for Json -> Either String a; I personally prefer to introduce type synonyms only when they significantly aid readability. It's not clear seeing Decoder a that it's a function, and so I would need to look up the type synonym to see what it is. I don't think the readability argument is that strong in this case.

That said, I know that my preference isn't necessarily shared. Elm uses the Decoder type, for example.

DecoderFunction? It prettifies implementation module a lot

If we use type synonyms then Encoder and Decoder are good names. I only commented because we haven't used synonyms before, using just the actual type Json -> Either String a, and in I often prefer seeing the actual type to a synonym.

That said, these synonyms are pretty clear and they do make things prettier. For the time being lets keep the change in here, as I wouldn't mind it making it in to master.

FWIW I strongly agree with @thomashoneyman. In my experience, obscuring a function signature via a type synonym does more harm than good. Json -> Either String a is hardly more verbose than Decoder a, and it's 100% clearer.

Yea I also agree with Dave and Thomas. I think type synonyms (when used to improve readability) are most useful when they hide the "noise" of a type signature. I don't find the type Json -> Either String a to be a noisy. All of that info is vital to understanding the functions inputs and outputs (And the type signature is relatively short in general)

Sorry, don't agree

I imagine there are functions everywhere in this file https://github.com/purescript-contrib/purescript-argonaut-codecs/blob/f006470fde11f39d5b17f2f40d05e6af320d6471/src/Data/Argonaut/Decode/Decoders.purs

@garyb writes JsonCodec everywhere, not BasicCodec (Either JsonDecodeError) J.Json

https://github.com/garyb/purescript-codec-argonaut/blob/f8766cb1dcc3c80d712e6c72ce91624dede84038/src/Data/Codec/Argonaut.purs#L57

Another pro argument - will be easier to change error type from String to typed error, for us and for users

@srghma Would you mind making the synonym change in a separate PR so it can be discussed separately? That way I can merge this PR as the other changes are good to go.

BasicCodec is already 2 levels deep of synonym too 😄.

But those codec types aren't on the same level as Json -> Either String a, I would be fine with that as-is. They're more like the lens synonyms. I think F in Foreign is a similar case, and a mistake also, as again the underlying types are pretty simple without the synonym.

The example with F is good, ok, maybe I'm wrong, will revert

@thomashoneyman no need, I have changed my mind

@thomashoneyman done

thomashoneyman · 2020-05-26T17:41:38Z

src/Data/Argonaut/Encode/Implementation.purs

+encodeJsonUnit :: Encoder Unit
+encodeJsonUnit = const jsonNull
+
+encodeJsonJBoolean :: Encoder Boolean


I recognize that the instance was called JBoolean, but I think we should drop the J prefix and just use Boolean for this function, as well as for Number, String, and so on.

thomashoneyman · 2020-05-26T17:43:04Z

src/Data/Argonaut/Encode/Implementation.purs

+encodeJsonNonEmptyArray encoder = encodeJsonArray encoder <<< NEA.toArray
+
+encodeJsonNonEmpty_List :: forall a . (Encoder a) -> Encoder (NonEmpty List a)
+encodeJsonNonEmpty_List encoder (NonEmpty h t) = encodeJsonList encoder (h : t)


Similar comment here about seeing if we can just export a single encodeNonEmpty function -- the encodeJsonNonEmpty_List isn't a usual PureScript function name and it's unfortunate we have both NonEmptyList and NonEmpty List cases to cover.

thomashoneyman · 2020-05-26T17:43:52Z

src/Data/Argonaut/Encode/Implementation.purs

+encodeIdentity :: forall a . Encoder a -> Encoder (Identity a)
+encodeIdentity encoder (Identity a) = encoder a
+
+encodeJsonMaybe :: forall a . Encoder a -> Encoder (Maybe a)


I think we can drop the Json and name these encodeMaybe, encodeTuple, and so on, rather than encodeJsonMaybe, encodeJsonTuple, and so on.

thomashoneyman · 2020-05-26T17:46:02Z

src/Data/Argonaut/Encode.purs

@@ -1,10 +1,11 @@
 module Data.Argonaut.Encode
  ( module Data.Argonaut.Encode.Class
  , module Data.Argonaut.Encode.Combinators
+  , module Data.Argonaut.Encode.Implementation


Implementation to me sounds like an internal module that you wouldn't necessarily import into your code. What do you think of renaming the Implementation modules to be Encoders and Decoders instead?

module Data.Argonaut.Encode ( ... , module Data.Argonaut.Encode.Encoders )

module Data.Argonaut.Decode ( ... , module Data.Argonaut.Decode.Decoders )

…on function as it is done for decoders

srghma-old · 2020-05-27T08:34:36Z

tests should work if restarted

thomashoneyman

👍 I'll let this sit for another day or two in case anyone else has comments, but it looks good to me and thank you for the contribution!

srghma-old force-pushed the jsonDecodeError-with-implmenetation branch 6 times, most recently from e48415b to 7e57801 Compare May 3, 2020 09:00

srghma-old added 2 commits May 22, 2020 19:54

feat: decoder implementation that is not dependent on class -> move i…

8f47b19

…mplementation to Implementation module

feat: encoder implementation that is not dependent on class -> move i…

3c5c44b

…mplementation to Implementation module

srghma-old force-pushed the jsonDecodeError-with-implmenetation branch from 7e57801 to 3c5c44b Compare May 22, 2020 17:45

thomashoneyman reviewed May 26, 2020

View reviewed changes

srghma-old mentioned this pull request May 26, 2020

why is L.insertAt 0 h t is used? #78

Closed

thomashoneyman linked an issue May 26, 2020 that may be closed by this pull request

why is L.insertAt 0 h t is used? #78

Closed

srghma-old added 2 commits May 27, 2020 11:08

refactor: review notes -> rename Implementation to Decoders/Encoders

92af096

refactor: review notes -> encodeJsonXXX to encodeXXX, remove encodeJs…

f006470

…on function as it is done for decoders

feat: review notes -> remove Decoder/Encoder types

d3dd3d9

thomashoneyman approved these changes May 29, 2020

View reviewed changes

thomashoneyman merged commit 2ad2147 into purescript-contrib:master May 31, 2020

srghma-old mentioned this pull request Sep 7, 2020

headers → opts purescript-graphqlclient/purescript-graphqlclient#18

Merged

srghma-old mentioned this pull request Nov 19, 2020

update to argonaut 7 and affjax 11 slamdata/purescript-lunapark#13

Merged

decoder implementation that is not dependent on class -> move implementation to Implementation module #74

decoder implementation that is not dependent on class -> move implementation to Implementation module #74

Uh oh!

Conversation

srghma-old commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this pull request do?

Uh oh!

srghma-old commented May 3, 2020

Uh oh!

thomashoneyman commented May 8, 2020

Uh oh!

thomashoneyman commented May 8, 2020

Uh oh!

srghma-old commented May 8, 2020

Uh oh!

garyb commented May 9, 2020

Uh oh!

srghma-old commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomashoneyman commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomashoneyman commented May 15, 2020

Uh oh!

srghma-old commented May 15, 2020

Uh oh!

srghma-old commented May 15, 2020

Uh oh!

srghma-old commented May 22, 2020

Uh oh!

thomashoneyman May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srghma-old May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomashoneyman May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyb May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srghma-old commented May 2, 2020 •

edited

Loading

srghma-old commented May 15, 2020 •

edited

Loading

thomashoneyman commented May 15, 2020 •

edited

Loading

thomashoneyman May 26, 2020 •

edited

Loading

srghma-old May 26, 2020 •

edited

Loading

thomashoneyman May 26, 2020 •

edited

Loading

garyb May 28, 2020 •

edited

Loading

thomashoneyman left a comment •

edited

Loading