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

Switch to MonadIO #6

Merged
merged 1 commit into from
Aug 26, 2023
Merged

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Aug 18, 2023

Switches all exposed actions to MonadIO, which makes it easy to use with various monad transformers and plethora of other libraries. Slight theoretical breaking change, but benign in practice, hence major version bump.

~This PR depends on #4 ~

@GianlucaGuarini
Copy link
Owner

GianlucaGuarini commented Aug 18, 2023

Could you please add more info about this change? Subclassing the IO Monad makes the code harder to read and increases its complexity. This project is meant to be used as standalone cli so I don't get the point about the polymorphic libs support. Thanks

@lehins
Copy link
Contributor Author

lehins commented Aug 18, 2023

I used it as a library within RIO monad, which is great for building applications. However, anytime you need to use an actual IO within any other monad that is based on IO you are forced to use liftIO, which is a bit of an inconvenience.

It doesn't have to be RIO, if my application uses a stack of transformers and I'd like to interact with a user within any of my functions, problem is the same. Switching it to MonadIO simplifies things.

Here is a concrete example:

writeToFileWithConfirmation
  :: (HasLogFunc env, MonadReader env m, MonadIO m)
  => FilePath
  -> Text
  -> m ()
writeToFileWithConfirmation filePath contents = do
  reportAlreadyExists <- doesFileExist filePath
  proceed <-
    if reportAlreadyExists
      then
        confirmWithDefault
          ("File with name: " <> show filePath <> " already exists! Overwrite?")
          False
      else pure True
  when proceed $ do
    writeBinaryFile filePath $ encodeUtf8 contents
    logInfo $ "Wrote Report to: " <> display (T.pack filePath)

Note that I didn't have to do any lifting.

The important part all of the actions that were switched to MonadIO will still work just fine in IO

@GianlucaGuarini
Copy link
Owner

After a bit of research I still don't get why the MonadIO subclassing should add better compatibility with other libs.

This article recommends splitting the monads into smaller semantic components that can be combined into transformers to avoid the MondadIO overhead.

Could you please add more examples (before and after the patch) to show how this change might improve the use of this lib? Thank you

@lehins
Copy link
Contributor Author

lehins commented Aug 18, 2023

This is how the example above would look like if I was using functions that are not using MonadIO:

writeToFileWithConfirmation
  :: (HasLogFunc env, MonadReader env m, MonadIO m)
  => FilePath
  -> Text
  -> m ()
writeToFileWithConfirmation filePath contents = do
  reportAlreadyExists <- liftIO $ doesFileExist filePath
  proceed <-
    if reportAlreadyExists
      then
        liftIO $ confirmWithDefault
          ("File with name: " <> show filePath <> " already exists! Overwrite?")
          False
      else pure True
  when proceed $ do
    liftIO $ writeBinaryFile filePath $ encodeUtf8 contents
    logInfo $ "Wrote Report to: " <> display (T.pack filePath)

Note that you can't restrict m to IO in the above function because logging imposes: MonadReader, so you have to call liftIO.

For current examples we are only talking about MonadIO, but for more complex examples there is potentially a need for MonadUnliftIO too. It is a common pattern in Haskell ecosystem, so much so that MonadIO is provided by base.

That being said. Feel free to close this PR if you don't see the benefit. It does not impact me in any way, I doubt that I will be an avid user of this library :) I just suggested this change, because it is good practice.

@lehins
Copy link
Contributor Author

lehins commented Aug 18, 2023

FYI, the article "MonadIO Considered Harmful" is just an opinion of one person, don't take it too seriously 😉

Copy link
Owner

@GianlucaGuarini GianlucaGuarini left a comment

Choose a reason for hiding this comment

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

Thank you @lehins . After a bit of research I understood the nature of this CR.
I have learnt a lot with it. I have just a small question but I am ready to merge it and bump a major release


-- | Ask a confirm falling back to a default value if no answer will be provided
confirmWithDefault :: String -> Bool -> IO Bool
confirmWithDefault :: MonadIO m => String -> Bool -> m Bool
confirmWithDefault question defaultAnswer = do
Copy link
Owner

Choose a reason for hiding this comment

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

Can't we wrap the whole function in a liftIO call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it makes no difference, really. I've wrapped the whole function.

@lehins
Copy link
Contributor Author

lehins commented Aug 23, 2023

@GianlucaGuarini I am really glad you found this PR useful. The whole concept of MonadIO is omnipresent in Haskell ecosystem. So, it is certainly a good thing to be familiar with.

I've addressed your comment too.

@GianlucaGuarini GianlucaGuarini merged commit 69b0ae4 into GianlucaGuarini:main Aug 26, 2023
1 check passed
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.

2 participants