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

Generic infix operators? #10

Closed
mikmart opened this issue Nov 4, 2023 · 9 comments
Closed

Generic infix operators? #10

mikmart opened this issue Nov 4, 2023 · 9 comments

Comments

@mikmart
Copy link

mikmart commented Nov 4, 2023

I'm working on an experimental package monad to provide generics and associated operators for monads. When looking for existing implementations, I came across your very clean implementation of the Maybe monad here.

Would you be interested in coordinating to use generic versions of fmap(), bind() and join() to get infix pipe operators? Since you already have the method implementations in {maybe} (with the same names even), it should be quite straightforward:

library(monad) # pak::pak("mikmart/monad")

# Signatures are not currently compatible
wrap <- \(fn) \(m, f, ...) fn(m, f, ...)
S7::method(fmap, S7::new_S3_class("maybe")) <- wrap(maybe::fmap)
S7::method(bind, S7::new_S3_class("maybe")) <- wrap(maybe::bind)
S7::method(join, S7::new_S3_class("maybe")) <- \(m) maybe::join(m)

# %>>% = fmap generic operator
# %>-% = bind generic operator
maybe::just(1) %>>% `+`(1) %>>% `*`(2)
#> Just
#> [1] 4
maybe::just(1) %>-% \(x) maybe::nothing()
#> Nothing
maybe::nothing() %>>% `+`(1) %>>% `*`(2)
#> Nothing
maybe::nothing() %>-% \(x) maybe::just(1)
#> Nothing
@armcn
Copy link
Owner

armcn commented Nov 4, 2023

Yeah, do you want to submit a PR for it? If you write the code, I can do the tests and documentation.

Are you planning to implement other monads in your package or is it only generics and operators? I agree that a unified package would be good to have, but it would likely only appeal to users who have a background in other FP languages. {maybe} is an attempt to make the syntax as familiar as possible to R/tidyverse programmers.

@mikmart
Copy link
Author

mikmart commented Nov 4, 2023

Sounds good. I'll see what I can put together. It'll also be a bit of a learning experience with S7. What do you think about the dependency structure? I assume it should be Suggests: monad?

With {monad} I started off thinking I would include some implementations too, but now I'm leaning towards just having it be the generic operators. Then implementing classes in other packages could either add optional support (requiring users to explicitly load the operators), or built-in support by re-exporting the operators. Especially with the sum types it's nice to have a dedicated namespace. Like for example result::ok() and result::err() feels a lot more appealing than monad::ok() or monad::result_ok().

@armcn
Copy link
Owner

armcn commented Nov 4, 2023

Now that I've thought a bit more, wouldn't it make more sense for monad to depend on maybe (and other monad packages)? I'm thinking of it more like an extension package (eg. ggplot or shiny extension). You would control the function wrappers, and users can opt in at any time. If you create tests that fail if the monad packages change their APIs you would be notified by a revdepcheck before the breaking change was on cran.

@mikmart
Copy link
Author

mikmart commented Nov 5, 2023

Yeah maybe. I've been mulling it over, and I keep going back and forth. I'm not a fan of the testing burden that comes with that, but then again it's only fmap, bind and join that would need to be tested.

I think the ideal situation would be that the implementing package exports the methods with an Enhances: monad because it doesn't really cause any extra testing burden. The package must have these methods anyway if it provides a monadic class, and there is really nothing to test with registering them with {monad}. There is of course the correctness of the infix operator implementations that monad adds, but that's for monad to test and isn't specific to any implementing class.

That said, the exporting should be made to be really simple for the implementing packages. And I don't think that's the case with S7 at the moment. I made a branch with what would be required for {maybe}, and I still had some weird behaviour with it. I also couldn't work out a way around the Imports: S7 which I understand would not be something you'd want to add at this point.

That's why I'm now considering that perhaps {monad} should just provide good old S3 generics. Then all you would need in {maybe} would be Enhances: monad and these simple registrations:

#' @exportS3Method monad::fmap
fmap.maybe <- function(m, f, ...) {
  maybe_map(m, f, ...)
}

#' @exportS3Method monad::bind
bind.maybe <- function(m, f, ...) {
  and_then(m, f, ...)
}

#' @exportS3Method monad::join
join.maybe <- function(m) {
  maybe_flatten(m)
}

@mikmart
Copy link
Author

mikmart commented Nov 5, 2023

Using S3 in monad might limit its future possibilities though. And I don't think I've considered those carefully enough at this stage. So the best approach right now is likely, as you suggested, to have the method registrations in monad.

@armcn
Copy link
Owner

armcn commented Nov 5, 2023

The maybe type probably should be an S7 object since the data contained in it is meant to be opaque. Is S7 not recommended as a CRAN dependency yet? According to the CRAN page for S7 there are no reverse dependencies. https://cloud.r-project.org/web/packages/S7/index.html

@mikmart
Copy link
Author

mikmart commented Nov 5, 2023

I think it's still quite early days for S7, as the first CRAN release was only a few weeks ago. I'm sure they would appreciate feedback from early adopters though.

@mikmart
Copy link
Author

mikmart commented Nov 5, 2023

I've gone ahead and added the fmap(), bind() and join() method registrations for maybe to {monad} as wrappes that call the equivalents in {maybe}. When figuring out what to test there, I ended up deciding to implement and export expectations for the functor and monad laws that should be satisfied when implementing these methods, and just added tests to verify that maybe satisfies the laws. I've documented that here: https://mikmart.github.io/monad/reference/maybe.html

I'm quite happy with how this has turned out, and at the moment I don't think there's a need to suggest any changes to {maybe}. Perhaps at some point in the future (once {monad} is on CRAN and the scope is well established) it might be appropriate for you to consider deprecating maybe::bind() etc. with a message to direct users who like that interface to use the {monad} generics. That way there would not be any namespace collisions if both packages are loaded.

Happy to close this issue if you don't see any further need to keep it open.

@armcn
Copy link
Owner

armcn commented Nov 6, 2023

Yeah that makes sense to me. Interested to see where your package goes in the future!

@armcn armcn closed this as completed Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants