Add maximumOn1, and minimumOn1 functions for Foldable1(no default imp… by rektrex · Pull Request #312 · kowainik/relude

rektrex · 2020-07-03T13:57:51Z

Resolves #306

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.

I don't know if it's possible to define a default implementation for maximumOn1 and minimumOn1. I've not added them to the MINIMAL pragma for now. I'll try to provide default implementations if there are any suggestions.

…lementation)

hint-man

Do you know why your PR is still not approved? Because I chose not to approve it. But they will.

src/Relude/Extra/Foldable1.hs

rektrex · 2020-07-03T15:08:23Z

I am sorry, I wasn't paying proper attention. I have changed the typeclass constraint.

chshersh

Thanks for your contribution @rektrex! I have some suggestions on how to improve the implementation further 🙂

chshersh · 2020-07-04T09:14:15Z

src/Relude/Extra/Foldable1.hs

    {-# INLINE minimum1 #-}

+    maximumOn1 :: Ord b => (a -> b) -> NonEmpty a -> a
+    maximumOn1 func = foldl1' (cmpOn func)


I think you don't need to pass func as an argument to cmpOn since func is already in scope for cmpOn

chshersh · 2020-07-04T09:14:52Z

src/Relude/Extra/Foldable1.hs

+    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


Could you, please add type signature to cmpOn? It usually improves code readability to add type signatures to functions in where 🙂

chshersh · 2020-07-04T09:15:46Z

src/Relude/Extra/Foldable1.hs

+                        GT -> a
+                        _  -> b


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 -> ... _ ->

chshersh · 2020-07-04T09:16:12Z

src/Relude/Extra/Foldable1.hs

+    minimumOn1 func = foldl1' (cmpOn func)
+      where
+        cmpOn p a b = case p a `compare` p b of
+                        LT -> a
+                        _  -> b


The same suggestions apply to minimumOn1 🙂

chshersh · 2020-07-04T09:17:04Z

src/Relude/Extra/Foldable1.hs

+    >>> maximumOn1 sin (32 :| [64, 8, 128, 16])
+    8.0
+    -}
+    maximumOn1 :: Ord b => (a -> b) -> f a -> a


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.

It should be possible to provide a default implementation if Foldable1 had a foldr1. I can try opening a PR adding foldr1 to Foldable1.

For now, you can implement those functions using toNonEmpty

…toNonEmpty

chshersh · 2020-07-10T10:03:22Z

src/Relude/Extra/Foldable1.hs

+    >>> maximumOn1 sin (32 :| [64, 8, 128, 16])
+    8.0


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.

chshersh · 2020-07-10T10:03:58Z

src/Relude/Extra/Foldable1.hs

+    16.0
+    -}
+    minimumOn1 :: Ord b => (a -> b) -> f a -> a
+    minimumOn1 f = minimumOn1 f . toNonEmpty


Let's add {-# INLINE #-} pragmas to default implementations of maximumOn1 and minimumOn1

chshersh

Looks good 👍

rektrex · 2020-07-10T13:53:50Z

Thank you.

add maximumOn1, and minimumOn1 functions for Foldable1(no default imp…

2b9f142

…lementation)

rektrex requested review from chshersh and vrom911 as code owners July 3, 2020 13:57

hint-man bot reviewed Jul 3, 2020

View reviewed changes

rektrex added 2 commits July 3, 2020 19:42

correct documentation, and apply hint-man's suggestions [ci skip]

bb8dc28

fix typeclass constraint and remove ConstrainedClassMethods extension

c6a4832

chshersh reviewed Jul 4, 2020

View reviewed changes

rektrex added 2 commits July 4, 2020 16:15

add maximumOn1, and minimumOn1 to MINIMAL pragma for

7784525

provide default implementations for maximumOn1, and minimumOn1 using …

c64dd0e

…toNonEmpty

chshersh suggested changes Jul 10, 2020

View reviewed changes

change maximumOn1, and minimumOn1 examples

c38b7bd

chshersh approved these changes Jul 10, 2020

View reviewed changes

chshersh merged commit 48d2160 into kowainik:master Jul 10, 2020

chshersh assigned rektrex Jul 10, 2020

chshersh added the extra Relude.Extra label Jul 10, 2020

Uh oh!

Conversation

rektrex commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

HLint

General

Uh oh!

hint-man bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rektrex commented Jul 3, 2020

Uh oh!

chshersh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chshersh left a comment

Choose a reason for hiding this comment

Uh oh!

rektrex commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rektrex commented Jul 3, 2020 •

edited

Loading