Stop using fromJust #339

treeowl · 2019-11-12T21:13:28Z

Get rid of all but one use of fromJust.

Fixes #301

treeowl · 2019-11-12T21:13:55Z

Here's my best guess about what this stuff is supposed to do. Please review carefully.

treeowl · 2019-11-12T21:35:25Z

hedgehog/src/Hedgehog/Internal/Gen.hs

@@ -256,8 +259,7 @@ runGenT size seed (GenT m) =
 --
 evalGen :: Size -> Seed -> Gen a -> Maybe (Tree a)
 evalGen size seed =
-  fmap (fmap Maybe.fromJust) .
-  Tree.filter Maybe.isJust .
+  Tree.mapMaybe id .


This is a substantial change. Please be especially careful to check that it makes sense!

Provided Tree.mapMaybe works then this looks like it would be fine.

* Get rid of all but one use of `fromJust`. * Expose `mapMaybe`-like functions along with `filter`-like ones. Fixes hedgehogqa#301

jacobstanley · 2019-11-14T15:47:35Z

hedgehog/hedgehog.cabal

+  other-modules:
+    -- This has nothing to do with Hedgehog proper, and it's
+    -- too trivial to bother exposing.
+    Hedgehog.Internal.Predicate


I agree with the comment, but would like to maintain a policy of always exposing all internal modules

#339 (comment), most likely we don't even need to introduce a new module.

jacobstanley · 2019-11-14T16:21:53Z

I added a stupid unit test just to mess around with it in the repl, looks pretty good to me

jacobstanley · 2019-11-14T16:22:21Z

Just have a few style guide things, will make some suggestions

jacobstanley · 2019-11-14T16:22:52Z

hedgehog/src/Hedgehog/Internal/Gen.hs

@@ -1270,34 +1271,38 @@ ensure p gen = do
 --   forever. If we trigger these limits then the whole generator is discarded.
 --
 filter :: (MonadGen m, GenBase m ~ Identity) => (a -> Bool) -> m a -> m a
-filter p gen0 =
+filter p = mapMaybe (fromPred p)


Suggested change

filter p = mapMaybe (fromPred p)

filter p =

mapMaybe (fromPred p)

jacobstanley · 2019-11-14T16:23:37Z

hedgehog/src/Hedgehog/Internal/Gen.hs

-        else
-          try (k + 1)
+        case p x of
+          Just _ -> withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen


Suggested change

Just _ -> withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen

Just _ ->

withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen

jacobstanley · 2019-11-14T16:23:47Z

hedgehog/src/Hedgehog/Internal/Gen.hs

-          try (k + 1)
+        case p x of
+          Just _ -> withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen
+          Nothing -> try (k + 1)


Suggested change

Nothing -> try (k + 1)

Nothing ->

try (k + 1)

jacobstanley · 2019-11-14T16:23:56Z

hedgehog/src/Hedgehog/Internal/Gen.hs

  in
    try 0

 filterT :: MonadGen m => (a -> Bool) -> m a -> m a
-filterT p gen0 =
+filterT p = mapMaybeT (fromPred p)


Suggested change

filterT p = mapMaybeT (fromPred p)

filterT p =

mapMaybeT (fromPred p)

jacobstanley · 2019-11-14T16:24:09Z

hedgehog/src/Hedgehog/Internal/Gen.hs

-        else
-          try (k + 1)
+        case p x of
+          Just _ -> withGenT (mapGenT (Tree.mapMaybeT p)) gen


Suggested change

Just _ -> withGenT (mapGenT (Tree.mapMaybeT p)) gen

Just _ ->

withGenT (mapGenT (Tree.mapMaybeT p)) gen

jacobstanley · 2019-11-14T16:24:17Z

hedgehog/src/Hedgehog/Internal/Gen.hs

-          try (k + 1)
+        case p x of
+          Just _ -> withGenT (mapGenT (Tree.mapMaybeT p)) gen
+          Nothing -> try (k + 1)


Suggested change

Nothing -> try (k + 1)

Nothing ->

try (k + 1)

moodmosaic · 2019-11-14T16:24:41Z

hedgehog/src/Hedgehog/Internal/Predicate.hs

+-- | Convert a Boolean predicate to something suitable for use
+-- with @mapMaybe@-like functions. The @wizards@ package calls this
+-- @ensure@. @protolude@ and @relude@ generalize from 'Maybe' to
+-- arbitrary 'Control.Applicative.Alternative' and call it @guarded@.


(Extreme nitpick) - need to push those comments on line (8-10) two space deeper, then add an empty -- line, if we want this to be consistent with all the other commented stuff, like so:

-- | Convert a Boolean predicate to something suitable for use -- with @mapMaybe@-like functions. The @wizards@ package calls this -- @ensure@. @protolude@ and @relude@ generalize from 'Maybe' to -- arbitrary 'Control.Applicative.Alternative' and call it @guarded@. --

(Absolutely not a blocking issue, of course!) 📨

Not sure if we even want a dedicated module for this, as we can just declare this in both Gen and Tree (but not expose it). Then, if we need to use it on a third module, we can consider the rule of three and move it to its own module.

It's just that Hedgehog.Internal.Predicate looks like a utility module that we don't really need (yet)...

jacobstanley · 2019-11-14T16:26:33Z

hedgehog/src/Hedgehog/Internal/Predicate.hs

+module Hedgehog.Internal.Predicate
+  ( fromPred
+  ) where


can't do multiline suggestions but

- module Hedgehog.Internal.Predicate - ( fromPred + module Hedgehog.Internal.Predicate ( + fromPred ) where

jacobstanley · 2019-11-14T16:26:52Z

hedgehog/src/Hedgehog/Internal/Tree.hs

+      Nothing -> mxs
+      Just x ->
+        case p x of
+          Just x' -> [Tree (Node x' mxs)]


Suggested change

Just x' -> [Tree (Node x' mxs)]

Just x' ->

[Tree (Node x' mxs)]

jacobstanley · 2019-11-14T16:27:01Z

hedgehog/src/Hedgehog/Internal/Tree.hs

+      Just x ->
+        case p x of
+          Just x' -> [Tree (Node x' mxs)]
+          Nothing -> mxs


Suggested change

Nothing -> mxs

Nothing ->

mxs

jacobstanley · 2019-11-14T16:27:09Z

hedgehog/src/Hedgehog/Internal/Tree.hs


 -- | Returns a tree containing only elements that match the predicate.
 --
 --   When an element does not match the predicate its node is replaced with
 --   'empty'.
 --
 filterT :: (Monad m, Alternative m) => (a -> Bool) -> TreeT m a -> TreeT m a
-filterT p m =
+filterT p = mapMaybeT (fromPred p)


Suggested change

filterT p = mapMaybeT (fromPred p)

filterT p =

mapMaybeT (fromPred p)

jacobstanley · 2019-11-14T16:28:23Z

Sorry for all the style nits, appreciate the contribution!

moodmosaic · 2019-11-18T15:26:54Z

@treeowl would you like me or @jacobstanley to apply those few style nits above? Happy to do so if you're busy. After that we should be good to go and merge this 🚀

treeowl · 2019-11-18T15:38:35Z

Go ahead. I'm sluggish today.

…

On Mon, Nov 18, 2019, 10:26 AM Nikos Baxevanis ***@***.***> wrote: @treeowl <https://github.com/treeowl> would you like me or @jacobstanley <https://github.com/jacobstanley> to apply those few style nits above? Happy to do so if you're busy. After that we should be good to go and merge this 🚀 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#339?email_source=notifications&email_token=AAOOF7KVY2YW653LWVL3DPTQUKX4BA5CNFSM4JMJEQQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEKZ6EA#issuecomment-555065104>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOF7MWHYFLNVUJEJUO5SLQUKX4BANCNFSM4JMJEQQA> .

See https://github.com/hedgehogqa/haskell-hedgehog/blob/a475c98240d1499103afc3970ef461b67288d3d8/STYLE_GUIDE.md

See hedgehogqa#339 (comment)

treeowl force-pushed the nix-fromJust branch from 094ffc7 to fe4b046 Compare November 12, 2019 21:28

treeowl commented Nov 12, 2019

View reviewed changes

Stop using fromJust

c6b0bff

* Get rid of all but one use of `fromJust`. * Expose `mapMaybe`-like functions along with `filter`-like ones. Fixes hedgehogqa#301

treeowl force-pushed the nix-fromJust branch from fe4b046 to c6b0bff Compare November 14, 2019 00:06

jacobstanley reviewed Nov 14, 2019

View reviewed changes

Added stupid mapMaybe test for Tree

392b846

jacobstanley reviewed Nov 14, 2019

View reviewed changes

moodmosaic reviewed Nov 14, 2019

View reviewed changes

jacobstanley reviewed Nov 14, 2019

View reviewed changes

jacobstanley force-pushed the nix-fromJust branch from 1b8b97f to 392b846 Compare November 14, 2019 16:29

moodmosaic added 2 commits November 19, 2019 00:20

Style nits

861f1db

See https://github.com/hedgehogqa/haskell-hedgehog/blob/a475c98240d1499103afc3970ef461b67288d3d8/STYLE_GUIDE.md

Move fromPred right next to its usage

fc40cc1

See hedgehogqa#339 (comment)

jacobstanley approved these changes Nov 19, 2019

View reviewed changes

jacobstanley merged commit d63a71d into hedgehogqa:master Nov 19, 2019

moodmosaic mentioned this pull request Jan 23, 2020

evalGen errors on fromJust #342

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using fromJust #339

Stop using fromJust #339

treeowl commented Nov 12, 2019

treeowl commented Nov 12, 2019

treeowl Nov 12, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

moodmosaic Nov 14, 2019

jacobstanley commented Nov 14, 2019

jacobstanley commented Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

moodmosaic Nov 14, 2019

moodmosaic Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley Nov 14, 2019

jacobstanley commented Nov 14, 2019

moodmosaic commented Nov 18, 2019

treeowl commented Nov 18, 2019 via email

	filter p = mapMaybe (fromPred p)
	filter p =
	mapMaybe (fromPred p)

	Just _ -> withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen
	Just _ ->
	withGenT (mapGenT (Tree.mapMaybeMaybeT p)) gen

	filterT p = mapMaybeT (fromPred p)
	filterT p =
	mapMaybeT (fromPred p)

	Just _ -> withGenT (mapGenT (Tree.mapMaybeT p)) gen
	Just _ ->
	withGenT (mapGenT (Tree.mapMaybeT p)) gen

	Just x' -> [Tree (Node x' mxs)]
	Just x' ->
	[Tree (Node x' mxs)]

Stop using fromJust #339

Stop using fromJust #339

Conversation

treeowl commented Nov 12, 2019

treeowl commented Nov 12, 2019

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

jacobstanley commented Nov 14, 2019

jacobstanley commented Nov 14, 2019

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

jacobstanley commented Nov 14, 2019

moodmosaic commented Nov 18, 2019

treeowl commented Nov 18, 2019 via email