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

Add instance for Ratio? #113

Open
kindaro opened this issue Jul 26, 2023 · 5 comments
Open

Add instance for Ratio? #113

kindaro opened this issue Jul 26, 2023 · 5 comments

Comments

@kindaro
Copy link

kindaro commented Jul 26, 2023

I want to encode and decode values of type Ratio. It is not supported yet. My offer is to encode and decode the same way as Aeson:

λ encode (3 % 5 ∷ Ratio Int)
"{\"numerator\":3,\"denominator\":5}"

My case specifically is related to argonaut-aeson-generic and purescript-bridge, so it is important to me that the encoding matches the Haskell side of my code. Since PureScript does not support orphan instances, I have no other way of making stuff work.

I can write a patch in no time.

@kindaro
Copy link
Author

kindaro commented Jul 27, 2023

@garyb
Copy link
Member

garyb commented Jul 28, 2023

There was a bit of discussion on the discord about this, and the point was brought up that adding dependencies on js libraries is something that might want to be avoided for this library.

My 2p: it's probably fine, since argonaut-core is very tied to JS anyway. purescript-json is intended to replace it - it's heavily based on argonaut, but choices have been made to make it more reasonable for other backends to implement. (Also it's not published yet, but will be doing so this weekend).

@thomashoneyman
Copy link
Contributor

Hi @kindaro — I'm no longer a maintainer of this library, but you may be able to find someone in the #contrib channel on Discord who can look at this with an eye to merging. However, some thoughts. First in brief, then a bit longer:

In brief, I'm OK with adding this dependency and a instance for Ratio, although I'm always uneasy to add more dependencies to an already-large library, and I doubt that adding this instance will solve all your problems. First, because this library makes no promises to be Aeson-compatible, and second, because you will probably encounter more types you want instances for and we may not be open to adding some of them to this library. At that point you'll need a new approach anyways. Personally, I recommend forking this library to explicitly work with purescript-bridge and aeson.


My case specifically is related to argonaut-aeson-generic and purescript-bridge, so it is important to me that the encoding matches the Haskell side of my code.

I recommend being careful here. Argonaut does not promise to match Aeson, though it borrows heavily from that library. I don't expect that all instances in this library are the same as the Aeson forms, and they may diverge in the future. If you need to have a JSON library that exactly matches Aeson for use with purescript-bridge, then I think you should fork this library or create a new one with that explicit goal (that would also let you write any additional instances you want).

Since PureScript does not support orphan instances, I have no other way of making stuff work.

Yes, this is a problem with type class codec libraries like argonaut-codecs. They're a convenient base but are a huge pain when you need to diverge from the "canonical" implementation they provide or you need to add instances of a class you don't own to a type you don't own. (In my applications I use codec-argonaut for this very reason — sometimes I need more than one way to serialize a given type).

The most common thing to do is write a new app-specific class in your codebase for dealing with JSON. The Decoders and Encoders modules exist to help you write your own classes by providing default implementations:
https:/purescript-contrib/purescript-argonaut-codecs/blob/main/src/Data/Argonaut/Decode/Decoders.purs
https:/purescript-contrib/purescript-argonaut-codecs/blob/main/src/Data/Argonaut/Encode/Encoders.purs

You can copy/paste the Argonaut classes into your application:
https:/purescript-contrib/purescript-argonaut-codecs/blob/main/src/Data/Argonaut/Decode/Class.purs
https:/purescript-contrib/purescript-argonaut-codecs/blob/main/src/Data/Argonaut/Encode/Class.purs
and either use the Decoders and Encoders modules as the implementation, adding any more instances you want, or you can skip the dependency on argonaut-codecs altogether and just copy the relevant implementations you want into your codebase.

There are other, worse options: for example, you can newtype Ratio, and then write an instance for your newtype.

There was a bit of discussion on the discord about this, and the point was brought up that adding dependencies on js libraries is something that might want to be avoided for this library. My 2p: it's probably fine, since argonaut-core is very tied to JS anyway.

That's a good point; with purescript-json available, if we won't be switching to it instead of purescript-argonaut-core in this library, then maybe Argonaut just accepts that it is tied to the JS backend and JS dependencies don't matter.

@kindaro
Copy link
Author

kindaro commented Jul 29, 2023

Thank you @garyb @thomashoneyman.

I am going to be building my stuff from my own fork for now. Whether this feature gets merged or not, in the long term I shall need to talk to @eskimor (maintainer of purescript-bridge) and @coot (maintainer of purescript-argonaut-aeson-generic) about whether it makes sense to make an official fork that promises to be compatible with Aeson.

@coot
Copy link

coot commented Jul 30, 2023

I am no longer the maintainer of purescript-argonaut-aeson-generic. You should speak with @peterbecich or @StephenWakely.

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

No branches or pull requests

4 participants