Make Object use an opaque Textmap interface #865
Conversation
|
I still didn't add the |
Boarders
force-pushed
the
textmap
branch
2 times, most recently
from
ec32861
to
463b365
Compare
|
Do you think there is any value in also making this abstract in terms of the |
|
Let's do with concrete |
|
FWIW, feel free to squash my commit once things starts to look ok. |
Boarders
force-pushed
the
textmap
branch
9 times, most recently
from
93bb3a1
to
9738cb5
Compare
|
Apologies if this led to a bunch of CI spam. |
Boarders
force-pushed
the
textmap
branch
5 times, most recently
from
5dfae44
to
c4d793b
Compare
src/Data/Aeson/Types/ToJSON.hs
Outdated
| liftToJSON g _ = Object . fmap g | ||
|
|
||
| -- liftToEncoding :: forall a. (a -> Encoding) -> ([a] -> Encoding) -> TM.HashMap k a -> Encoding | ||
| liftToEncoding g _ = case toJSONKey of |
There was a problem hiding this comment.
the same issue as previously with liftToJSON.
There was a problem hiding this comment.
You'll have to let me know if what I wrote is the expected behaviour.
|
@phadej any ideas what to do with the panic on GHC 8.0.2 - I don't have access to a local copy and failed to quickly build it. |
|
ignore it for now, i guess what it suggests will do the trick, but it's easier to test locally (i'll do) EDIT: i want also to check what it is simplifying there, doesn't feel right it hits the limit |
|
Ah, #if __GLASGOW_HASKELL__ <= 710 && __GLASGOW_HASKELL__ >= 706
-- Work around a compiler bug
{-# OPTIONS_GHC -fsimpl-tick-factor=300 #-}
#endifbump that to 400, let's see if that's enough :) |
Co-authored-by: Callan McGill <callan.mcgill@gmail.com>
|
Thank you for the guidance @phadej. Looks like we got it to green. |
Boarders
changed the title
|
|
||
| -- | Convert a 'TextMap' to a 'HashMap'. | ||
| toHashMap :: TextMap v -> HashMap Text v | ||
| toHashMap = unTextMap |
There was a problem hiding this comment.
I didn't add inline anywhere, I think, in general, it is best practice to add inline when a benchmark actually proves it is worthwhile and not before, but I am open to changing it if others agree.
There was a problem hiding this comment.
I'm thinking how get rid (or at least discourage the usage) of this function. We don't want to have HashMap intermediates, if they are not necessary i.e. we are literally parsing into HashMap / serializing from HashMap...
There was a problem hiding this comment.
Let me investigate if these functions are really needed. For the instances for HashMap itself we can probably use a fold which will be more representative of an abstract interface.
There was a problem hiding this comment.
They are fine for now. Probably the "pain" will clearer when there is alternative implementation.
There was a problem hiding this comment.
To clarify: we would like to avoid rebuilding a *Map when the representation is the same. nice to have.
There was a problem hiding this comment.
There might be some way to not expose it as part of the interface but add a re-write rule for this representation or something like that.
No description provided.