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

work with any MonadIO fixing #104 #105

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Conversation

KommuSoft
Copy link
Contributor

Fixing #104

@jgm
Copy link
Owner

jgm commented Nov 29, 2022

@tarleb what do you think of this change?

One query. Instead of

 class ToJSONFilter m a where
   toJSONFilter :: a -> m ()

would it make sense to do

class ToJSONFilter a where
  toJSONFilter :: forall m. MonadIO m => a -> m ()

? (I suppose maybe the former is more flexible, as it allows one to define instances for non MonadIO monads, and even non-monad functors.)

Comment on lines +106 to 110
instance (Walkable a Pandoc) => ToJSONFilter IO (a -> a) where
toJSONFilter f = BL.getContents >>=
BL.putStr . encode . (walk f :: Pandoc -> Pandoc) . either error id .
eitherDecode'

Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need this? After all, IO is an instance of MonadIO, so this is already covered by lines 111-115, isn't it?
Ditto for the other IO instance below.

@tarleb
Copy link
Contributor

tarleb commented Nov 30, 2022

I guess I'd prefer the forall version over the multiparam typeclass, but I don't feel strongly about it.

@jgm
Copy link
Owner

jgm commented Nov 30, 2022

Is there any particular reason favoring the forall approach?

@tarleb
Copy link
Contributor

tarleb commented Nov 30, 2022

I left out my reasoning intentionally, as I don't have a good answer for that. The universally quantified variant seems cleaner and more focused to me. The extra flexibility that comes with the multiparam typeclass variant probably won't be necessary for most users.

@jgm
Copy link
Owner

jgm commented Nov 30, 2022

I think most users won't touch this at all, or even know about it.

Seems to me that non-MonadIO instances are conceivable -- e.g. for Conduit or pipes or any streaming abstraction that can receive input and give output.

@KommuSoft
Copy link
Contributor Author

The original reason to do this was to also work for example with a StateT IO Int Pandoc, and thus can use this when working with the toJsonFilter (walkM someFunc) with someFunc :: Pandoc -> StateT IO Int Pandoc for example. We can work with evalStateT 0 (walkM someFunc) and use that with toJsonFilter, but that requires to initialize the state before the walkM and the state is "gone" after the walkM then. It would be nice if toJsonFilter could thus work with StateT IO Int Pandoc as well, to process this. The forall will likely not allow that, since it means it can convert it to any MonadIO.

@jgm
Copy link
Owner

jgm commented Dec 1, 2022

It should work fine either way with StateT IO Int Pandoc -- something that works for all MonadIO instances will work for this one.

@KommuSoft
Copy link
Contributor Author

Yes, but if someFunc thus is an Pandoc -> StateT IO Int Pandoc, then the toJsonFilter needs to be able to handle this, with the current implementation, it only handles a Pandoc -> Pandoc or a Pandoc -> IO Pandoc.

@jgm
Copy link
Owner

jgm commented Dec 2, 2022

Maybe I'm misunderstanding. If we have

class ToJSONFilter a where
  toJSONFilter :: forall m. MonadIO m => a -> m ()

Then toJSONFilter could indeed be applied to someFunc which has type Pandoc -> StateT IO Int (), which is an instance of forall m. MonadIO m => a -> m(). It could not be applied to Pandoc -> StateT IO Pandoc, but neither could your interface

 class ToJSONFilter m a where
   toJSONFilter :: a -> m ()

because () doesn't match Pandoc.

@jgm jgm merged commit 6a571b2 into jgm:master Jan 13, 2023
@jgm
Copy link
Owner

jgm commented Jan 13, 2023

I'm going to merge this and remove the unnecessary IO instance in a second commit.

jgm added a commit that referenced this pull request Jan 13, 2023
This is covered already by the MonadIO instance.
Revises #105.
jgm added a commit that referenced this pull request Aug 5, 2023
Previously a pure function `a -> a` could only be promoted to
a filter in IO. Now we allow it to work with any instance of
MonadIO.  (This adds to #105.)
jgm added a commit that referenced this pull request Aug 5, 2023
This went missing after my ill-considered revision to #105,
commit 183af9d .

See jgm/pandoc#8976.
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

Successfully merging this pull request may close these issues.

3 participants