[#100] Add Validation data type to Extra modules #106

mauriciofierrom · 2018-10-15T05:58:05Z

Resolves #100

Add Validation data type to Extra modules.

Checklist:

HLint

I've changed the exposed interface (add new reexports, remove reexports, rename reexported things, etc.).
- I've updated hlint.dhall accordingly to my changes (add new rules for the new imports, remove old ones, when they are outdated, etc.).
- I've generated the new .hlint.yaml file (see this instructions).

General

I've updated the CHANGELOG with the short description of my latest changes.
All new and existing tests pass.
I keep the code style used in the files I've changed (see style-guide for more details).
I've used the stylish-haskell file.
My change requires the documentation updates.
- I've updated the documentation accordingly.
I've added the [ci skip] text to the docs-only related commit's name.

mauriciofierrom · 2018-10-15T06:49:48Z

It seems the tests are failing because Data.Bifoldable and Data.Bitraversable are not exported in GHC 8.0.2.

module Relude.Foldable.Reexport
       ( module Data.Foldable
       , module Data.Traversable
#if MIN_VERSION_base(4,10,0)
       , module Data.Bifoldable
       , module Data.Bitraversable
#endif
       ) where

vrom911 · 2018-10-15T07:31:07Z

@mauriciofierrom , thanks for your contribution!

Regarding the CI issue, I guess, the same CPP method (as in the Relude.Foldable.Reexport module) should be used in here 🙂

chshersh

Looks good! I have several comments. And, as @vrom911 pointed out, we should use -XCPP here to have Bifoldable and Bitraversable instances only for higher base versions

chshersh · 2018-10-15T07:46:58Z

src/Relude/Extra/Validation.hs

+
+{- |
+Copyright: (c) 2014 Chris Allen, Edward Kmett
+           (c) 2018 Kowainic


Minor typo here, should be Kowainik 🙂

chshersh · 2018-10-15T07:52:45Z

src/Relude/Extra/Validation.hs

+    liftA2 _ (Failure e) (Failure e') = Failure (e <> e')
+    liftA2 _ (Failure e) (Success _) = Failure e
+    liftA2 _ (Success _) (Failure e) = Failure e
+    liftA2 f (Success a) (Success a') = Success (f a a')


Just minor comment, but I think that instead of a' this variable could be named b to match type more precisely. The same trick cannot be done with e and e' because type variable is e in both cases. But I think that we can have e1 and e2 or el (e-left) and er (e-right). It's just my personal dislike of ' in names because according to common Haskell convention ' is usually used at the end of functions to mark it as strict (like foldl and foldl').

chshersh · 2018-10-15T07:53:40Z

src/Relude/Extra/Validation.hs

+    (*>) = liftA2 (flip const)
+
+    (<*) :: Validation e a -> Validation e b -> Validation e a
+    (<*) = liftA2 const


These are the default definitions of <* and *>. I wonder, if we can write them more efficiently if we do pattern-matching explicitly? 🤔

They are both defined based on liftA2, which is a custom implementation and actually uses pattern-matching explicitly, so I thought perhaps it was best to just leave these methods for readibility's sake. I can change it anyway, WDYT?

Let's keep the default implementation here as well 👍

chshersh · 2018-10-15T07:55:50Z

src/Relude/Extra/Validation.hs

+    fold (Failure _) = mempty
+
+    foldMap :: Monoid m => (a -> m) -> Validation e a -> m
+    foldMap f = foldr (mappend . f) mempty


I think foldMap can be written more efficiently more via explicit pattern-matching

Kind of the same thing as with <* but in terms of foldr. I can change it anyway, WDYT?

Well, for <* and *> we're kinda hope for the inlining and optimizations and implementing <* as liftA2 const is not a big deal. But for foldMap optimizations might not trigger because the implementation is more difficult. So it's better to pattern-match here explicitly as well.

chshersh · 2018-10-15T07:57:15Z

src/Relude/Extra/Validation.hs

+validationToEither :: Validation e a -> Either e a
+validationToEither x = case x of
+                         Failure e -> Left e
+                         Success a -> Right a


I'd prefer using -XLabmdaCase language extension and write it like:

validationToEither = \case Failure e -> Left e Success a -> Right a

chshersh · 2018-10-15T07:57:41Z

src/Relude/Extra/Validation.hs

+
+-- | Transform an 'Either' into a 'Validation'.
+eitherToValidation :: Either e a -> Validation e a
+eitherToValidation x = case x of


Same here as for validationToEither

chshersh · 2018-10-15T07:58:14Z

src/Relude/Extra/Validation.hs

+    {-# INLINE fmap #-}
+    {-# INLINE (<$) #-}
+
+instance Semigroup e => Applicative (Validation e) where


I think it would be really great to add couple doctest tests to this instance to show different cases 👍

Sounds good 👍

chshersh · 2018-10-15T07:58:51Z

src/Relude/Extra/Validation.hs

+       , eitherToValidation
+       ) where
+
+import Data.Function (const)


I think relude exports const, no need to have redundant import

chshersh · 2018-10-15T07:59:21Z

src/Relude/Extra/Validation.hs

@@ -0,0 +1,150 @@
+{-# LANGUAGE InstanceSigs #-}


After reading this module I'm now even more sure that -XInstanceSigs should be enabled by default!

chshersh · 2018-10-15T07:59:34Z

src/Relude.hs

@@ -89,6 +89,7 @@ every module in your package by modifying your "Prelude" file:
  @newtype@.
 * __"Relude.Extra.Tuple"__: functions for working with tuples.
 * __"Relude.Extra.Type"__: functions for inspecting and working with types.
+* __"Relude.Extra.Validation"__: 'Validation' data type.


Thanks a lot for updating the documentation! ❤️

mauriciofierrom · 2018-10-15T13:01:21Z

It seems liftA2 is only available from base-4.10.0 so I need to use the CPP conditional here too. I defined (<*) & (*>) using it, so I either include the three functions in the conditional and leave the default implementations for (<*) & (*>), which are based on (<$), or rewrite them like @chshersh suggested. WDYT?

chshersh · 2018-10-16T09:27:42Z

I defined (<) & (>) using it, so I either include the three functions in the conditional and leave the default implementations for (<) & (>), which are based on (<$), or rewrite them like @chshersh suggested. WDYT?

Hmm... Yeah, having the default implementation for *> and <* might not help because it won't work when there's no liftA2 inside Applicative and we can't implement liftA2 efficiently. In that case let's write <* and *> with manual pattern-matching.

chshersh · 2018-10-16T09:30:11Z

src/Relude/Extra/Validation.hs

+-- | Examples:
+--
+-- >>> let fa = Success (*3) :: Validation Text (Int -> Int)
+-- >>> let ga = Success (*4) :: Validation Text (Int -> Int)


For example purposes it's better to use [Text] instead of just Text. It's not a good idea to use Text as a monoidal error, so let's not show non-idiomatic examples in our documentation examples.

chshersh · 2018-10-16T09:32:29Z

src/Relude/Extra/Validation.hs

+    {-# INLINE fmap #-}
+    {-# INLINE (<$) #-}
+
+-- | Examples:


Maybe it's a good idea to write this as ==== __Examples__ to make them expandable 🤔
Not sure how it will look like.

Also, IMO, for multiline haddock comments it's better to use: the following style

{- | -}

so you won't need to add -- at the beginning of each line.

The proposed expandable formatting works, but looks like this in Firefox and Chrome:

It currently looks like this:

WDYT? 🤔

I think that non-expandable is better 🙂

chshersh · 2018-10-16T09:33:06Z

src/Relude/Extra/Validation.hs

+instance Traversable (Validation e) where
+    traverse :: Applicative f => (a -> f b) -> Validation e a -> f (Validation e b)
+    traverse f (Success a) = Success <$> f a
+    traverse _ (Failure e) = pure (Failure e)


Maybe we can also implement sequenceA here?

vrom911

Amazing 🔥

chshersh

Great change!

vrom911 requested review from ghallak, vrom911, chshersh and willbasky October 15, 2018 07:26

vrom911 assigned mauriciofierrom Oct 15, 2018

vrom911 added new Bring something new into library (add function or type or interface) Hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 15, 2018

vrom911 added this to In progress in #2: Hacktoberfest (October, 2018) via automation Oct 15, 2018

chshersh reviewed Oct 15, 2018

View reviewed changes

mauriciofierrom added 2 commits October 15, 2018 07:00

[#100] Add Validation data type to Extra modules

c7445a1

Most changes as per code review

8c3b41a

chshersh reviewed Oct 16, 2018

View reviewed changes

mauriciofierrom added 3 commits October 16, 2018 07:06

More changes as per code review

1f5fe16

Change liftA2 inline pragma location

baf670c

Add sequenceA

0960e8a

#2: Hacktoberfest (October, 2018) automation moved this from In progress to Reviewer approved Oct 16, 2018

vrom911 approved these changes Oct 16, 2018

View reviewed changes

chshersh approved these changes Oct 16, 2018

View reviewed changes

chshersh merged commit 3e99f9c into kowainik:master Oct 16, 2018

#2: Hacktoberfest (October, 2018) automation moved this from Reviewer approved to Done: PR's Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#100] Add Validation data type to Extra modules #106

[#100] Add Validation data type to Extra modules #106

mauriciofierrom commented Oct 15, 2018

mauriciofierrom commented Oct 15, 2018

vrom911 commented Oct 15, 2018

chshersh left a comment

chshersh Oct 15, 2018

chshersh Oct 15, 2018

chshersh Oct 15, 2018

mauriciofierrom Oct 15, 2018

chshersh Oct 16, 2018

chshersh Oct 15, 2018

mauriciofierrom Oct 15, 2018

chshersh Oct 16, 2018

chshersh Oct 15, 2018

chshersh Oct 15, 2018

chshersh Oct 15, 2018

mauriciofierrom Oct 15, 2018

chshersh Oct 15, 2018

chshersh Oct 15, 2018

chshersh Oct 15, 2018

mauriciofierrom commented Oct 15, 2018

chshersh commented Oct 16, 2018

chshersh Oct 16, 2018

chshersh Oct 16, 2018

mauriciofierrom Oct 16, 2018 •

edited

chshersh Oct 16, 2018

chshersh Oct 16, 2018

vrom911 left a comment

chshersh left a comment

[#100] Add Validation data type to Extra modules #106

[#100] Add Validation data type to Extra modules #106

Conversation

mauriciofierrom commented Oct 15, 2018

Checklist:

HLint

General

mauriciofierrom commented Oct 15, 2018

vrom911 commented Oct 15, 2018

chshersh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciofierrom commented Oct 15, 2018

chshersh commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciofierrom Oct 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrom911 left a comment

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

mauriciofierrom Oct 16, 2018 •

edited