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

makeMockable incompatible with DefaultSignatures #25

Closed
TravisCardwell opened this issue Dec 1, 2021 · 2 comments
Closed

makeMockable incompatible with DefaultSignatures #25

TravisCardwell opened this issue Dec 1, 2021 · 2 comments

Comments

@TravisCardwell
Copy link
Contributor

It is common to use the DefaultSignatures extension to provide default implementations of class methods. When using a class as an abstraction layer over I/O, default implementations can be provided for MonadIO instances. This reduces boilerplate when the class has more than one instance (using MonadIO), and it allows the allows the implementation to be written alongside the method signatures, making it easier to maintain. The example in the documentation could be implemented as follows:

class Monad m => MonadFilesystem m where
  readFile :: FilePath -> m String

  default readFile :: MonadIO m => FilePath -> m String
  readFile = liftIO . readFile

  writeFile :: FilePath -> String -> m ()

  default writeFile :: MonadIO m => FilePath -> String -> m ()
  writeFile = (liftIO .) . writeFile

Unfortunately, makeMockable fails with an error like the following:

/path/to/test/Spec.hs:25:1: error:
    Cannot derive MockT because Demo.MonadFileSystem has unmockable methods.
   |
25 | makeMockable [t|MonadFileSystem|]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This error is raised when instExtraMembers is not empty. It is set in makeInstance, where members is processed using getMethod. Perhaps support for DefaultSignatures could be added by refactoring this code so that default members are ignored?

@TravisCardwell
Copy link
Contributor Author

One can filter the members before processing them as follows:

diff --git a/src/Test/HMock/TH.hs b/src/Test/HMock/TH.hs
index 56b786e..cbc1c84 100644
--- a/src/Test/HMock/TH.hs
+++ b/src/Test/HMock/TH.hs
@@ -177,7 +177,7 @@ makeInstance ::
   [Dec] ->
   Q Instance
 makeInstance options ty cx tbl ps m members = do
-  processedMembers <- mapM (getMethod ty m tbl) members
+  processedMembers <- mapM (getMethod ty m tbl) $ filter isApplicableMember members
   (extraMembers, methods) <-
     partitionEithers <$> zipWithM memberOrMethod members processedMembers
   return $
@@ -190,6 +190,10 @@ makeInstance options ty cx tbl ps m members = do
         instExtraMembers = extraMembers
       }
   where
+    isApplicableMember :: Dec -> Bool
+    isApplicableMember DefaultSigD{} = False
+    isApplicableMember _ = True
+
     memberOrMethod :: Dec -> Either [String] Method -> Q (Either Dec Method)
     memberOrMethod dec (Left warnings) = do
       when (mockVerbose options) $ mapM_ reportWarning warnings

If other types of members should be ignored as well, those cases can be easily added to the isApplicableMember function.

What do you think? If this looks good, I am happy to submit a pull request.

@cdsmith
Copy link
Owner

cdsmith commented Dec 1, 2021

Yes, this looks like the right thing to do. A PR would be great, if you have the time. Even better if you can add a test to https://github.com/cdsmith/HMock/blob/main/test/Classes.hs, but I'm happy to do that, too.

@cdsmith cdsmith closed this as completed in b073d0e Dec 3, 2021
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