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.

Sign up for GitHub

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

Jump to bottom

Consider offering a Prism into an Object's KeyMap #38

Closed
RyanGlScott opened this issue Oct 9, 2021 · 3 comments · Fixed by #42
Closed

Consider offering a Prism into an Object's KeyMap #38

RyanGlScott opened this issue Oct 9, 2021 · 3 comments · Fixed by #42

Comments

@RyanGlScott
Copy link
Member

In aeson-2.0.0.0 and later, Object uses a KeyMap rather than directly exposing a HashMap. See haskell/aeson#866. In light of this, the current type of _Object (Prism' t (HashMap Text Value)) is not ideal—ideally, it would focus on a KeyMap instead. Some possibilities are:

  1. Keep _Object's type as is, but offer another method of type Prism' t (KeyMap Value) alongside it. (_ObjectKeyMap, perhaps?)
  2. Just change the type of _Object to Prism' t (KeyMap Value) . This would require a breaking API change.

Either option will likely require waiting a bit until we can require aeson >= 2 unconditionally.
@RyanGlScott RyanGlScott mentioned this issue Oct 9, 2021
Closed
RyanGlScott added a commit that referenced this issue Oct 9, 2021
@RyanGlScott
Allow building with aeson-2.0.0.0
52d213b 
`aeson-2.0.0.0` changes `Object`'s field from a `HashMap` to a new `KeyMap`
type. This patch adapts `lens-aeson` to the new API. h/t to
phadej/aeson-optics@ff31ca0, on which I based
this patch.

We may want to consider changing the type of `_Object` later (#38), but that
should probably wait until we can depend on `aeson >= 2` unconditionally.

Fixes #37.
@RyanGlScott RyanGlScott mentioned this issue Oct 9, 2021
Merged
RyanGlScott added a commit that referenced this issue Oct 9, 2021
@RyanGlScott
Allow building with aeson-2.0.0.0
ae47809 
`aeson-2.0.0.0` changes `Object`'s field from a `HashMap` to a new `KeyMap`
type. This patch adapts `lens-aeson` to the new API. h/t to
phadej/aeson-optics@ff31ca0, on which I based
this patch.

We may want to consider changing the type of `_Object` later (#38), but that
should probably wait until we can depend on `aeson >= 2` unconditionally.

Fixes #37.
RyanGlScott added a commit that referenced this issue Oct 9, 2021
@RyanGlScott
Allow building with aeson-2.0.0.0 (#39)
98c9d81 
`aeson-2.0.0.0` changes `Object`'s field from a `HashMap` to a new `KeyMap`
type. This patch adapts `lens-aeson` to the new API. h/t to
phadej/aeson-optics@ff31ca0, on which I based
this patch.

We may want to consider changing the type of `_Object` later (#38), but that
should probably wait until we can depend on `aeson >= 2` unconditionally.

Fixes #37.
@sjshuck sjshuck mentioned this issue Mar 5, 2022
Closed
@sjshuck
Copy link

sjshuck commented Mar 7, 2022 •
edited
Loading

FWIW, I'm in favor of (2), namely redefining

_Object :: AsValue t => Prism' t Object

since

  • Object is already a thing and that's the normal naming pattern for prisms associated with constructors, and
  • the type synonym Object itself broke compatibility, and
  • people probably use key much more than _Object, so we might not break that much anyway.

@RyanGlScott
Copy link
Member Author

Now that Stackage Nightly includes aeson-2.0.*, perhaps it's time to take action on option (2).

RyanGlScott added a commit that referenced this issue Mar 7, 2022
@RyanGlScott
Require aeson-2.0.*, lens-5.0.* or greater
3af3250 
A part of a fix for #38.
RyanGlScott added a commit that referenced this issue Mar 7, 2022
@RyanGlScott
Use KeyMap instead of HashMap when dealing with Objects
5260216 
In `aeson-2.0.*`, `Object`'s field is of type `KeyMap Value` rather than
`HashMap Text Value`. Now that Stackage Nightly includes `aeson-2.0.*`, it's
high time that we updated the `lens-aeson` API to mirror this change. This most
notably affects the type of `_Object`, which also requires updating the types
of `key` and `members` as a consequence.

In order to make the doctests continue to work, I found myself needing to add
`Wrapped` and `Rewrapped` instances for `KeyMap`. In these instances, I treat
`KeyMap v` as a wrapper around `[(Key, v)]`, where the order of the key-value
pairs in the list is not stable.

Fixes #38.
@RyanGlScott RyanGlScott mentioned this issue Mar 7, 2022
Merged
@RyanGlScott
Copy link
Member Author

See #42.

@ekmett ekmett closed this as completed in #42 Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use KeyMap instead of HashMap when dealing with Objects lens/lens-aeson
2 participants
@RyanGlScott @sjshuck