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

[#82] Remove maybeT, add diOptional #101

Merged
merged 3 commits into from
Oct 4, 2018

Conversation

willbasky
Copy link
Collaborator

Resolves: #82

@willbasky willbasky added bug Something isn't working refactoring labels Oct 2, 2018
@willbasky willbasky self-assigned this Oct 2, 2018
@chshersh chshersh added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Oct 3, 2018
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great implementation!

@@ -25,6 +25,8 @@ The change log is available [on GitHub][2].
Improve `Double` generator for property-based tests.
* Add support for GHC 8.6.1.
Drop support for GHC 8.0.2.
* [#82](https://github.com/kowainik/tomland/issues/82):
Remove `maybeT`. Add `diOptional`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also bump up cabal-version in .cabal file as well? I see that it's still 0.4.1 for a very long time...

@@ -16,8 +16,8 @@ module Toml.Bi.Combinators
, arrayOf

-- * Combinators
, diOptional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The O letter should be small, so it's just dioptional. Standard naming convention for profunctor functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since function works with general Codec, it should go to the Toml.Bi.Monad module

maybeT :: forall a . (Key -> TomlCodec a) -> Key -> TomlCodec (Maybe a)
maybeT converter key = let codec = converter key in Codec
{ codecRead = (Just <$> codecRead codec) `catchError` handleNotFound
diOptional :: (Functor r, Functor w, Applicative w) => Codec r w c a -> Codec r w (Maybe c) (Maybe a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functor is a superclass of Applicative, so it's enough to have only Applicative w in the constraints.

, codecWrite = \case
Nothing -> pure Nothing
Just v -> codecWrite codec v >> pure (Just v)
Just c -> Just <$> codecWrite c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! You've just implemented Traversable instance for Maybe. If you look at this instance, you probably might notice how to implement codecWrite in more concise way:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, maybe someday I will implement somewhat function for relude :)

maybeT converter key = let codec = converter key in Codec
{ codecRead = (Just <$> codecRead codec) `catchError` handleNotFound
diOptional :: (Functor r, Functor w, Applicative w) => Codec r w c a -> Codec r w (Maybe c) (Maybe a)
diOptional Codec {..} = Codec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no space between Codec and {..}

@vrom911
Copy link
Member

vrom911 commented Oct 3, 2018

Travis fails with an error for tasty-quickcheck for Cabal. Seems like this package has breaking change in the newest version and one of our dependencies (which depends on the tasty-quickcheck because we don't 🙂 ) didn't have the upper bounds for that. I will investigate this..

@chshersh
Copy link
Contributor

chshersh commented Oct 3, 2018

@vrom911 Thank you very much for working on this! 💖

@vrom911
Copy link
Member

vrom911 commented Oct 3, 2018

It seems it fixed itself. Version 2.12.6.1 of the QuickCheck just a few hours after the 2.12.6, so guess the issue was in there, cause it works now 🙂

@willbasky
Copy link
Collaborator Author

willbasky commented Oct 3, 2018

While I slept the problem with QuickCheck was solved :)

@willbasky willbasky force-pushed the willbasky/82-Change-maybeT-with-dioptional branch from 76d406a to 9f3ca9e Compare October 3, 2018 05:20
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@vrom911 vrom911 merged commit 384d840 into master Oct 4, 2018
@vrom911 vrom911 deleted the willbasky/82-Change-maybeT-with-dioptional branch October 4, 2018 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Hacktoberfest https://hacktoberfest.digitalocean.com/ refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants