k0001 / safe-money

Test project
5 stars 1 forks source link

Simplify JSON serialization #10

Closed k0001 closed 6 years ago

k0001 commented 7 years ago

In our JSON serialization, we don't' really need that first "Dense" or "Discrete" value.

> Data.Aeson.encode (32 :: Dense "EUR")
"[\"Dense\",\"EUR\",32,1]"
> Data.Aeson.encode (4 :: Discrete "XAU" "gram")
"[\"Discrete\",\"XAU\",31103477,1000000,4]"

This change is backwards incompatible.

k0001 commented 6 years ago

@ocharles since other than me, you are the only user of safe-money I know of: Do you mind if I do this? Are you storing raw JSON serializations somewhere?

ocharles commented 6 years ago

Can you elaborate on the change? I'm not entirely understanding what's changed. I'll check out code base

On 9 Dec 2017 8:51 pm, "Renzo Carbonara" notifications@github.com wrote:

@ocharles https://github.com/ocharles since other than me, you are the only user of safe-money I know of: Do you mind if I do this? Are you storing raw JSON serializations somewhere?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/k0001/safe-money/issues/10#issuecomment-350504682, or mute the thread https://github.com/notifications/unsubscribe-auth/AABRjqZEe1dTHsDL6Jtj7pFG7NRPAAL8ks5s-vLPgaJpZM4PrvDu .

k0001 commented 6 years ago

@ocharles see the JSON serializations above as arrays? I am planning to drop the first element from both of them (i.e. the strings "Dense" and "Discrete") since it's not really necessary and makes the JSON bigger.

k0001 commented 6 years ago

To clarify: This change will only affect you if you are storing the current JSON serialization somewhere.

ocharles commented 6 years ago

I've only had a cursory glance, but from the looks of things - yes, we do use the FromJSON Dense instance. @rehno-lindeque, @HanStolpo wrote the code using it though, so I'm CCing them.

k0001 commented 6 years ago

It's OK if you use the FromJSON instance for ephemeral data that exists while the app is running. The problem would only arise if you were storing the generated JSON somewhere for later retrieval and expecting to be able to decode it at a later date.

But actually, I just had an idea: I'll make the FromJSON implementation support both the new format and the old format, while having the ToJSON instance generate only the new format. So... don't worry about a thing :)

ocharles commented 6 years ago

The problem would only arise if you were storing the generated JSON somewhere for later retrieval and expecting to be able to decode it at a later date.

Well, and any external clients that expect to be able to communicate with this Haskell application. Our Elm app posts to the Haskell application, so we would have to remember to update its JSON encoding.

I don't think we use ToJSON anywhere, so that would work for us.

k0001 commented 6 years ago

@ocharles I just made the change here https://github.com/k0001/safe-money/commit/89156d445cbca7d7c5b370cc896f5333a684014a

Agreed, if your client app is relying on the current ToJSON behavior, it'll need to change a bit. Sorry :(

rehno-lindeque commented 6 years ago

Thanks for the heads up @k0001!