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.

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

Swap order of type arguments for BiMap in match and arrayOf functions #95

Closed
chshersh opened this issue Oct 1, 2018 · 4 comments
Closed
Labels
codec Conversion between TOML and custom user data types Hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@chshersh
Copy link
Contributor

chshersh commented Oct 1, 2018

Currently we have the following combinator:

data BiMap a b = BiMap
    { forward  :: a -> Maybe b
    , backward :: b -> Maybe a
    }

prism :: (object -> Maybe field) -> (field -> object) -> BiMap object field
prism preview review = BiMap preview (Just . review)

But usually it's used with invert function to get required type:

_Url :: BiMap Text Source
_Url = Toml.invert $ Toml.prism matchUrl Url

We need to add combinator like invert . prism but I don't know proper name for it 😞 The name should be short because it's going to be used often.

@chshersh chshersh added codec Conversion between TOML and custom user data types Hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 1, 2018
@chshersh chshersh added this to To do in #2: Hacktoberfest (October, 2018) via automation Oct 1, 2018
@chshersh chshersh changed the title Add combinator similar to prism but vice versa Add combinator for inverted prism Oct 1, 2018
@jiegillet
Copy link
Collaborator

Why does the type need to be _Url :: BiMap Text Source rather than _Url :: BiMap Source Text?
Comparing :

Control.Lens.Prism._Left :: Prism (Either a c) (Either b c) a b
Toml._Left :: BiMap l (Either l r)

Control.Lens.Prism doesn't use an invert in the implementation.

@chshersh
Copy link
Contributor Author

chshersh commented Oct 6, 2018

@jiegillet The reason for this is because prisms in tomland has two uses cases:

  1. For array values:
    arrayOf :: forall a . Typeable a => BiMap AnyValue a -> Key -> TomlCodec [a]
  2. For parsing sum types with the help of match function
    match :: forall a . Typeable a => BiMap AnyValue a -> Key -> TomlCodec a

Though, as I can see now, those functions can take BiMap a AnyValue instead of BiMap AnyValue a and there won't be any need in having inverted prism. Even if this is a quite serious breaking change, I think this is the way to go. I'd rather prefer consistency with the rest of the Haskell ecosystem and not have extra function.

@chshersh chshersh changed the title Add combinator for inverted prism Swap order of type arguments for BiMap in match and arrayOf functions Oct 6, 2018
@jiegillet
Copy link
Collaborator

I'll try to do the change. It might take a while, may the compiler be with me :)

chshersh pushed a commit that referenced this issue Oct 11, 2018
* Reversed BiMap Anything a into BiMap a Anything

* Removed inverse function

* Added _Just, invert, cleaned up function names

* Added #106 to CHANGELOG

* Flipped _TextBy arguments

* Reference issues instead of PRs

* typo in CHANGELOG
@chshersh
Copy link
Contributor Author

Closed in #106

#2: Hacktoberfest (October, 2018) automation moved this from To do to Done: Issues Oct 11, 2018
jiegillet added a commit to jiegillet/tomland that referenced this issue Oct 12, 2018
vrom911 pushed a commit that referenced this issue Oct 25, 2018
* Added several BiMaps

* Modified .cabal file to include bytestring

* Flipped Bimap arguments following #95

* Added _LazyByteString

* Modified _Word and _Natural

* Added maybeInteger signatures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codec Conversion between TOML and custom user data types Hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
No open projects
Development

No branches or pull requests

2 participants