[Issue #95 ]Reversed BiMap Anything a into BiMap a Anything #106
Conversation
chshersh
added
codec
|
@jiegillet Wow, you continue to bring very good improvements to the package! Let me take a look at the original issue and the original example one more time very carefully to make sure that we really need to swap order of arguments. |
|
Please take your time. |
src/Toml/BiMap.hs
Outdated
| @@ -67,10 +69,6 @@ instance Cat.Category BiMap where | |||
| , backward = backward bc >=> backward ab | |||
| } | |||
|
|
|||
| -- | Inverts bidirectional mapping. | |||
| invert :: BiMap a b -> BiMap b a | |||
| invert (BiMap f g) = BiMap g f | |||
There was a problem hiding this comment.
It's still a good idea to have invert function, it's somewhat idiomatic.
| eitherT1 = Toml.match (Toml._Integer >>> Toml._Left) "either.Left" | ||
| <|> Toml.match (Toml._String >>> Toml._Right) "either.Right" | ||
| eitherT1 = Toml.match (Toml._Left >>> Toml._Integer) "either.Left" | ||
| <|> Toml.match (Toml._Right >>> Toml._String) "either.Right" |
There was a problem hiding this comment.
Okay, looks like changing types of arguments pays off. I like this order of composition 👍 Lessons learned: Kmett always right.
src/Toml/BiMap.hs
Outdated
| _TextToString :: BiMap Text String | ||
| _TextToString = iso T.unpack T.pack | ||
| _StringToText :: BiMap String Text | ||
| _StringToText = iso T.pack T.unpack |
There was a problem hiding this comment.
I have better idea regarding naming convention: let's remove To part from such prism. So it will be just: _StringText. It doesn't make much sense to me now to have To part because conversion is bidirectional.
CHANGELOG.md
Outdated
| @@ -6,6 +6,9 @@ The change log is available [on GitHub][2]. | |||
|
|
|||
| 0.5.0 | |||
| ===== | |||
|
|
|||
| * [#106](https://github.com/kowainik/tomland/pull/106) | |||
There was a problem hiding this comment.
Could, you, please, change the link to the pull request here to the link for corresponding issue? The issue number is more important. I somehow missed it once, and now everyone specifies link to the PR...
src/Toml/BiMap.hs
Outdated
| -- | Creates prism for 'Text' to 'AnyValue' bimap with custom functions | ||
| _TextBy :: (Text -> Maybe a) -> (a -> Text) -> BiMap AnyValue a | ||
| -- | Creates prism for 'Text' to 'AnyValue' with custom functions | ||
| _TextBy :: (Text -> Maybe a) -> (a -> Text) -> BiMap a AnyValue |
There was a problem hiding this comment.
Let's also swap order of arguments for _TextBy to be more consistent with the prism combinator
CHANGELOG.md
Outdated
| * [#106](https://github.com/kowainik/tomland/pull/106) | ||
| Swap fields in BiMaps for consistency with `lens` package. | ||
| * [#104](https://github.com/kowainik/tomland/pull/104) | ||
| * [#95](https://github.com/kowainik/tomland/issues/95 |
There was a problem hiding this comment.
Probably a typo here: missing )
A couple of thoughts:
mkAnyValueBiMap, I couldn't useprismsince the direction didn't work out, so I ended up usingBiMap. It works, but it's a bit blunt._Show. Same as the others?stack testruns fineHappy to discuss more.