Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement andM, orM, allM, and anyM in terms of &&^ and ||^ #193

Merged
merged 4 commits into from
Sep 21, 2019

Conversation

josephcsible
Copy link
Contributor

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.

src/Relude/Foldable/Fold.hs Outdated Show resolved Hide resolved
src/Relude/Foldable/Fold.hs Outdated Show resolved Hide resolved
@josephcsible josephcsible changed the title Implement andM et al. in terms of ifM Implement andM et al. in terms of &&^ Sep 21, 2019
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice code simplification and refactoring 👍 Thanks!

@josephcsible
Copy link
Contributor Author

Now that these functions are written like this, I wonder if it's worth adding monadic versions of All and Any that base's Data.Monoid has. Something like this:

newtype AllM m = AllM (m Bool)
instance Monad m => Semigroup (AllM m) where
  AllM x <> AllM y = AllM (x &&^ y)
instance Monad m => Monoid (AllM m) where
  mempty = AllM (pure True)

And then we could rewrite these all in terms of foldMap with them.

@chshersh
Copy link
Contributor

@josephcsible the behaviour of these functions indeed can be generalised using new newtypes. However, I prefer to use current explicit implementation via foldr and not introduce AllM and AnyM newtypes. There're multiple reasons for this:

  1. These newtypes don't look very useful outside of these 4 functions. Quick Hoogle search shows that they are not even present on Hackage, which means that they are not that common and idiomatic to be included in the standard library.
  2. One of the relude goals is to be minimalistic, which implies not introducing new abstractions without need. And since these 4 functions are exported by default, we can't just put AllM and AnyM in Extra modules.
  3. We can have these 2 newtypes locally without exporting. But I guess that using coerced foldMap vs current foldr implementation is the same in terms of performance. So no need to complicate code and make it more difficult for beginners to read and understand.
  4. If we reexport them by default, they steal names from the scope. And since these newtypes are not idiomatic, people can already use AnyM and AllM for some other stuff.

And I can add from myself that it's a bit unfortunate that App f Any and App f All don't have the desired behaviour of lazy monadic boolean operators...

@josephcsible
Copy link
Contributor Author

josephcsible commented Sep 21, 2019

Okay, I guess this can go in as-is then.

By the way, what is the type App that you're talking about? I see a lot of types with that name in Hoogle, but none look particularly relevant. EDIT: Oh, did you mean Ap?

@chshersh
Copy link
Contributor

@josephcsible Yeah, I mean Ap, Sorry, this was a typo 😞

Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring!

CHANGELOG.md Outdated Show resolved Hide resolved
@josephcsible josephcsible changed the title Implement andM et al. in terms of &&^ Implement andM, orM, allM, and anyM in terms of &&^ and ||^ Sep 21, 2019
Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@chshersh
Copy link
Contributor

CI is failing due to import issues. When CI pass, this PR is ready to be merged 🙂

@josephcsible
Copy link
Contributor Author

It passes now.

@chshersh chshersh merged commit c52e812 into kowainik:master Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants