Add maximumOn1, and minimumOn1 functions for Foldable1(no default imp… #312
Conversation
![hint-man[bot]](https://avatars.githubusercontent.com/in/20170?s=60&v=4){kind=small}
|
I am sorry, I wasn't paying proper attention. I have changed the typeclass constraint. |
src/Relude/Extra/Foldable1.hs
Outdated
| @@ -139,6 +155,22 @@ instance Foldable1 NonEmpty where | |||
| {-# INLINE maximum1 #-} | |||
| {-# INLINE minimum1 #-} | |||
|
|
|||
| maximumOn1 :: Ord b => (a -> b) -> NonEmpty a -> a | |||
| maximumOn1 func = foldl1' (cmpOn func) | |||
There was a problem hiding this comment.
I think you don't need to pass func as an argument to cmpOn since func is already in scope for cmpOn
src/Relude/Extra/Foldable1.hs
Outdated
| maximumOn1 :: Ord b => (a -> b) -> NonEmpty a -> a | ||
| maximumOn1 func = foldl1' (cmpOn func) | ||
| where | ||
| cmpOn p a b = case p a `compare` p b of |
There was a problem hiding this comment.
Could you, please add type signature to cmpOn? It usually improves code readability to add type signatures to functions in where 🙂
src/Relude/Extra/Foldable1.hs
Outdated
| GT -> a | ||
| _ -> b |
There was a problem hiding this comment.
The alignment here doesn't match our style guide. It looks like currently it depends on the length of the identifier on a previous line, but always should be the same. Like this:
cmpOn ...
GT -> ...
_ ->
src/Relude/Extra/Foldable1.hs
Outdated
| minimumOn1 func = foldl1' (cmpOn func) | ||
| where | ||
| cmpOn p a b = case p a `compare` p b of | ||
| LT -> a | ||
| _ -> b |
There was a problem hiding this comment.
The same suggestions apply to minimumOn1 🙂
| >>> maximumOn1 sin (32 :| [64, 8, 128, 16]) | ||
| 8.0 | ||
| -} | ||
| maximumOn1 :: Ord b => (a -> b) -> f a -> a |
There was a problem hiding this comment.
If there's no default implementation, those functions should be listed in the {-# MINIMAL #-} pragma. But I hoped that it should be possible to create a default implementation using foldr1 or something like that.
There was a problem hiding this comment.
It should be possible to provide a default implementation if Foldable1 had a foldr1. I can try opening a PR adding foldr1 to Foldable1.
There was a problem hiding this comment.
For now, you can implement those functions using toNonEmpty
src/Relude/Extra/Foldable1.hs
Outdated
| >>> maximumOn1 sin (32 :| [64, 8, 128, 16]) | ||
| 8.0 |
There was a problem hiding this comment.
I think this example is a bit difficult to understand and not that helpful. I can't calculate sin of any number in my head, so I can't verify that this function actually works by simply looking at the example. Also, the list is specified as integral numbers but the output is written as a Double due to overloaded numerals. I understand that, but it makes the example not so beginner-friendly.
I propose to change example to maximumOn1 by the abs function and have a list with positive and negative numbers, where the maximum by abs is some negative number.
| 16.0 | ||
| -} | ||
| minimumOn1 :: Ord b => (a -> b) -> f a -> a | ||
| minimumOn1 f = minimumOn1 f . toNonEmpty |
There was a problem hiding this comment.
Let's add {-# INLINE #-} pragmas to default implementations of maximumOn1 and minimumOn1
|
Thank you. |
Resolves #306
Checklist:
HLint
hlint.dhallaccordingly to my changes (add new rules for the new imports, remove old ones, when they are outdated, etc.)..hlint.yamlfile (see this instructions).General
stylish-haskellfile.[ci skip]text to the docs-only related commit's name.I don't know if it's possible to define a default implementation for
maximumOn1andminimumOn1. I've not added them to theMINIMALpragma for now. I'll try to provide default implementations if there are any suggestions.