Add 'either' combinator for TomlCodec #346

markus1189 · 2020-10-17T07:54:04Z

Implement the either combinator from #326

hint-man

There is no place for me here... I will choose the truth I like.

chshersh

@markus1189 Thanks for your contribution! Much appreciated 🙂
I left some comments on possible code improvements and some suggestions on how to move this forward ⏩

chshersh · 2020-10-17T15:03:08Z

src/Toml/Codec/Combinator/Custom.hs

@@ -10,6 +10,7 @@ See examples below of the situations you may need the following combinators.

 @since 1.3.0.0
 -}
+{-# LANGUAGE LambdaCase #-}


This extension is enabled in the tomland.cabal file by default in all modules, so no need to add it here 🙂

Suggested change

{-# LANGUAGE LambdaCase #-}

chshersh · 2020-10-17T15:03:50Z

src/Toml/Codec/Combinator/Custom.hs

@@ -20,16 +21,21 @@ module Toml.Codec.Combinator.Custom
 -- * Validation
 , validate
 , validateIf
+
+ -- * Transform


Let's rename the section to give some more meaning to it 🙂

Suggested change

-- * Transform

-- * With the errors reporting

chshersh · 2020-10-17T15:08:20Z

src/Toml/Codec/Combinator/Custom.hs

+either :: TomlCodec a -> TomlCodec (Either [TomlDecodeError] a)
+either (Codec r w) = Codec r' $ \case
+ Left x' -> pure . Left $ x'
+ Right x' -> fmap Right . w $ x'


This case looks like the Traversable instance for Either, so you can use a single function traverse instead of a manual case:

https://hackage.haskell.org/package/base-4.14.0.0/docs/src/Data.Traversable.html#line-240

You're totally right! 😉

chshersh · 2020-10-17T15:10:19Z

src/Toml/Codec/Combinator/Custom.hs

+either (Codec r w) = Codec r' $ \case
+ Left x' -> pure . Left $ x'
+ Right x' -> fmap Right . w $ x'
+ where r' = pure . validationToEither . r


Let's rename variables for clarity, and also use Success here for clarity. Just to make the code more maintainable 🙂

Suggested change

where r' = pure . validationToEither . r

where

newCodecRead :: TomlEnv (Either [TomlDecodeError] a)

newCodecRead = Success . validationToEither . oldCodecRead

Yes I agree, I moved out both newCodecRead and newCodecWrite

chshersh · 2020-10-17T15:11:55Z

test/Test/Toml/Codec/Combinator/Custom.hs

@@ -25,3 +26,6 @@ customSpec = describe "Combinator.Custom: Roundtrip tests" $ do
 codecRoundtrip "TextBy "
 (Toml.textBy (Text.pack . show) (first Text.pack . readEither . Text.unpack))
 Gen.genInt
+ codecRoundtrip "either "
+ (Toml.either . Toml.bool)
+ (Right <$> Gen.genBool)


Here you are testing the either combinator only on values generated with the constructor Right, but we actually want to test on both Left and Right. So, we need to generate either a list of decode errors or a different value (in your case it's Bool)

Hmmm I'm no longer sure if TomlCodec (Either [TomlDecodeError] a) is correct 🤔

We probably don't want to enode a Left [TomlDecodeError]... Maybe we should change either to:

either :: Codec i o -> Codec i (Either [TomlDecodeError] o)

and then we need to use something other than codecRoundtrip in the test 🤔

@markus1189 I think it is okay to have TomlCodec in type as TomlDecodeError works with TomlCodec 🙂
You can just change the generator to something with Gen.either.

src/Toml/Codec/Combinator/Custom.hs

markus1189

Thanks @chshersh for the friendly review! I left some comments in answer

markus1189 · 2020-10-17T16:25:19Z

src/Toml/Codec/Combinator/Custom.hs

@@ -10,6 +10,7 @@ See examples below of the situations you may need the following combinators.

 @since 1.3.0.0
 -}
+{-# LANGUAGE LambdaCase #-}


markus1189 · 2020-10-17T16:25:26Z

src/Toml/Codec/Combinator/Custom.hs

@@ -20,16 +21,21 @@ module Toml.Codec.Combinator.Custom
 -- * Validation
 , validate
 , validateIf
+
+ -- * Transform


markus1189 · 2020-10-17T16:25:45Z

src/Toml/Codec/Combinator/Custom.hs

+either :: TomlCodec a -> TomlCodec (Either [TomlDecodeError] a)
+either (Codec r w) = Codec r' $ \case
+ Left x' -> pure . Left $ x'
+ Right x' -> fmap Right . w $ x'


You're totally right! 😉

markus1189 · 2020-10-17T16:28:45Z

src/Toml/Codec/Combinator/Custom.hs

+either (Codec r w) = Codec r' $ \case
+ Left x' -> pure . Left $ x'
+ Right x' -> fmap Right . w $ x'
+ where r' = pure . validationToEither . r


Yes I agree, I moved out both newCodecRead and newCodecWrite

markus1189 · 2020-10-17T16:48:43Z

test/Test/Toml/Codec/Combinator/Custom.hs

@@ -25,3 +26,6 @@ customSpec = describe "Combinator.Custom: Roundtrip tests" $ do
 codecRoundtrip "TextBy "
 (Toml.textBy (Text.pack . show) (first Text.pack . readEither . Text.unpack))
 Gen.genInt
+ codecRoundtrip "either "
+ (Toml.either . Toml.bool)
+ (Right <$> Gen.genBool)


Hmmm I'm no longer sure if TomlCodec (Either [TomlDecodeError] a) is correct 🤔

We probably don't want to enode a Left [TomlDecodeError]... Maybe we should change either to:

either :: Codec i o -> Codec i (Either [TomlDecodeError] o)

and then we need to use something other than codecRoundtrip in the test 🤔

vrom911 · 2020-10-25T09:40:58Z

test/Test/Toml/Gen.hs

+genTValue = Gen.element [ TBool
+ , TInteger
+ , TDouble
+ , TText
+ , TZoned
+ , TLocal
+ , TDay
+ , THours
+ , TArray
+ ]


I guess we can use enumBounded here, if we derive Enum for TValue 🙂

vrom911 · 2021-02-08T08:46:31Z

Closing due to the inactivity.
Feel free to reopen if you would like to continue working on it 🙂

Add 'either' combinator for TomlCodec

bd4ab0d

hint-man bot approved these changes Oct 17, 2020

View reviewed changes

markus1189 marked this pull request as ready for review October 17, 2020 08:49

markus1189 requested review from chshersh and vrom911 as code owners October 17, 2020 08:49

chshersh reviewed Oct 17, 2020

View reviewed changes

chshersh assigned markus1189 Oct 17, 2020

chshersh added codec Conversion between TOML and custom user data types hacktoberfest-accepted Accept contributions during Hacktoberfest labels Oct 17, 2020

markus1189 commented Oct 17, 2020

View reviewed changes

markus1189 added 2 commits October 17, 2020 18:49

Address review, comment out test for now

6a33cbd

Finish up review

db2aed9

vrom911 reviewed Oct 25, 2020

View reviewed changes

vrom911 changed the base branch from master to main November 6, 2020 16:43

vrom911 closed this Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'either' combinator for TomlCodec #346

Add 'either' combinator for TomlCodec #346

markus1189 commented Oct 17, 2020 •

edited

Loading

hint-man bot left a comment

chshersh left a comment

chshersh Oct 17, 2020

markus1189 Oct 17, 2020

chshersh Oct 17, 2020

markus1189 Oct 17, 2020

chshersh Oct 17, 2020

markus1189 Oct 17, 2020

chshersh Oct 17, 2020

markus1189 Oct 17, 2020

chshersh Oct 17, 2020

markus1189 Oct 17, 2020

vrom911 Oct 19, 2020

markus1189 left a comment

markus1189 Oct 17, 2020

markus1189 Oct 17, 2020

markus1189 Oct 17, 2020

markus1189 Oct 17, 2020

markus1189 Oct 17, 2020

vrom911 Oct 25, 2020

vrom911 commented Feb 8, 2021

- where r' = pure . validationToEither . r
+ where
+ newCodecRead :: TomlEnv (Either [TomlDecodeError] a)
+ newCodecRead = Success . validationToEither . oldCodecRead

Add 'either' combinator for TomlCodec #346

Add 'either' combinator for TomlCodec #346

Conversation

markus1189 commented Oct 17, 2020 • edited Loading

hint-man bot left a comment

Choose a reason for hiding this comment

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

markus1189 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

vrom911 commented Feb 8, 2021

markus1189 commented Oct 17, 2020 •

edited

Loading