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

Use m ~ IO to improve type inference #23

Merged
merged 3 commits into from Sep 8, 2023
Merged

Conversation

ocharles
Copy link
Contributor

Currently, one has to use the hedgehog identity function to force the type of the test to be PropertyT IO (). A better way to do this is to instead use an equality constraint in the instance head. This means that when we typecheck it we encounter PropertyT m () as before, but we're now able to find an instance that matches. Refining the context will the force m to be IO and everything works out fine.

Currently, one has to use the `hedgehog` identity function to force the type of the test to be `PropertyT IO ()`. A better way to do this is to instead use an equality constraint in the instance head. This means that when we typecheck `it` we encounter `PropertyT m ()` as before, but we're now able to find an instance that matches. Refining the context will the force `m` to be `IO` and everything works out fine.
@ocharles
Copy link
Contributor Author

I think with this you could actually deprecate hedgehog and change the docs to not suggest pure () :: PropertyT m (), but I wanted to present just this change first.

Copy link
Member

@sol sol left a comment

Choose a reason for hiding this comment

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

This looks great 👍

@parsonsmatt can we get this into a release?

@sol
Copy link
Member

sol commented Jul 18, 2023

@parsonsmatt ping.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Looks good! Mind updating changelog and doing a major version bump?

@@ -142,8 +142,8 @@ hedgehog = id
-- of 'Example' for a function for more details.
--
-- @since 0.0.0.0
instance Example (PropertyT IO ()) where
type Arg (PropertyT IO ()) = ()
instance m ~ IO => Example (PropertyT m ()) where
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the main issue here is that end users won't be able to define their own instances of Example (PropertyT TheirNeatMonad ())

Which I'm pretty cool with, I think? But worth noting that it's a breaking change.

Wondering if putting an inference constraint on the (), too:

Suggested change
instance m ~ IO => Example (PropertyT m ()) where
instance (a ~ (), m ~ IO) => Example (PropertyT m a) where

This would forbid users from writing instances like:

instance (m ~ IO) => Example (PropertyT m (SqlPersistT m (TestT m ()))) where
  type Arg _ = SqlBackend

  -- run the property, 
  -- run the action,
  -- run the asserts

Which - idk - maybe it's good and proper to make that less magical?

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if putting an inference constraint on the (), too

I'm trying to mentally piece together what the tangible benefits would be. I guess you could put an undefined as the last statement of a do-block and things may still type check. You may also get better type errors at times.

From what I understand, adding this constrained does not change the picture fundamentally. But it could be nice to have.

For me, the essential thing is to constrain the base monad to IO. This is what I have been using for the last couple of months. Getting this on Hackage will remove a lot of friction points for me.

@sol
Copy link
Member

sol commented Sep 8, 2023

Looks good! Mind updating changelog and doing a major version bump?

Done as a separate PR (#27) to not unnecessarily cause conflicts.

@parsonsmatt parsonsmatt merged commit cd33d76 into hspec:master Sep 8, 2023
@ocharles ocharles deleted the patch-1 branch October 10, 2023 14:45
@ocharles
Copy link
Contributor Author

Thanks both!

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.

None yet

3 participants