lens / lens-aeson

Traversals and Prisms for Data.Aeson
MIT License
51 stars 18 forks source link

Add an `IsKey` class #44

Closed RyanGlScott closed 2 years ago

RyanGlScott commented 2 years ago

The _Key Iso will make it easier to convert to/from _Keys in idiomatic lens code. Inspired by https://github.com/lens/lens-aeson/pull/42#discussion_r820667720.

RyanGlScott commented 2 years ago

IsKey is simple enough to add. I also pondered adding something like IsKeyMap to handle conversions to/from KeyMap:

class IsKeyMap t where
  _KeyMap :: Iso' (t v) (KeyMap v)

However, I'm less clear on what the instances should be. One possibility is to do this:

instance IsKeyMap (Map Key) where
  _KeyMap = iso KM.fromMap KM.toMap

instance IsKeyMap (Map Text) where
  _KeyMap = iso KM.fromMapText KM.toMapText

instance IsKeyMap (HashMap Key) where
  _KeyMap = iso KM.fromHashMap KM.toHashMap

instance IsKeyMap (HashMap Text) where
  _KeyMap = iso KM.fromHashMapText KM.toHashMapText

However, that requires FlexibleInstances, which is somewhat awkward. An alternative would be:

instance IsKey k => IsKeyMap (Map k) where ...
instance IsKey k => IsKeyMap (HashMap k) where ...

That avoids FlexibleInstances, but at the cost of having to convert all of the ks in each Map/HashMap with _IsKey, which could potentially impose a runtime cost.

Due to this uncertainty, I left out IsKeyMap. If folks have an opinion on what the best design is, however, I can be convinced to add it.

ekmett commented 2 years ago

How well would this interoperate with OverloadedStrings?

sjshuck commented 2 years ago

With OverloadedStrings, these both result in Ambiguous type variable prevents the constraint IsKey from being solved:

x = _Key # "hello"  
y = "hello" ^. _Key 
sjshuck commented 2 years ago

I guess the other option was to have a bunch of concrete Iso's. In my PR I had thought to name them

keyText :: Iso' Key Text
keyString :: Iso' Key String
...
RyanGlScott commented 2 years ago

To be honest, my intended use case for this class wasn't to interact with OverloadedStrings. After all, if you have OverloadedStrings enabled, then "hello" will work as both a Text and a Key, so it will Just Work™ on most of the combinators in lens-aeson whose type changed to mention Key.

I think what I had in mind when you had a variable of type Text, in which case OverloadedStrings won't help you. Then again, I'm not sure how often this scenario will arise in practice. Perhaps it's more convenient just to use the conversion functions from Data.Aeson.Key directly.

sjshuck commented 2 years ago

Re: cases where OverloadedStrings won't help you: That makes a lot of sense. I would love to have access to some Iso's in situations where object keys are not strings literals but are being computed, in which case they almost certainly wouldn't be ambiguously typed. So I'm in favor of IsKey again.

But I would definitely add a note in the docs about its intended purpose, and/or warning about OverloadedStrings.

sjshuck commented 2 years ago

Re: IsKeyMap:

  1. I don't think hardly any APIs, for anything, accept maps. Lists of key-value pairs, maybe. (Which is now covered by Wrapped/Rewrapped.) But the actual utility of map conversion optics seems minimal, versus just using functions from aeson in those rare circumstances when needed. I'd be happy to be refuted though.
  2. _KeyMap seems complicated in both maintenance and use in a way that _Key doesn't.
RyanGlScott commented 2 years ago

I've expanded the documentation for _Key a bit to give an example of how to use it and to warn about its interaction with OverloadedStrings.

sjshuck commented 2 years ago

Looks great!