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

Rename n to g in Command #252

Merged
merged 3 commits into from
Apr 2, 2019
Merged

Conversation

endgame
Copy link
Contributor

@endgame endgame commented Mar 12, 2019

This is more of a "discussion PR" than a request to merge unchanged.

As the first argument to Command is almost always an instance of MonadGen, I believe g is a better mnemonic than n. I can pursue this change so that all MonadGen references are g, or only the state-machine-test-related ones, or somewhere in between.

I have also added to the haddock for Command. I found it hard to get my head around the way hedgehog's state machine testing support all fits together, and can create more documentation PRs if you like this sort of thing.

As the first argument to `Command` is almost always an instance of
`MonadGen`, `g` is a better mnemonic than `n`.
@gwils
Copy link
Contributor

gwils commented Mar 12, 2019

hedgehog-example needs that more restrictive bound since the example works with MonadBaseControl but does not work with MonadUnliftIO

@endgame
Copy link
Contributor Author

endgame commented Mar 12, 2019

I see. nixpkgs is now providing resourcet-1.2, which makes cabal new-build want to download and build resourcet-1.1.11. I will remove the bound relaxation from this PR. Advice for how to proceed wrt nixpkgs?

@gwils
Copy link
Contributor

gwils commented Mar 12, 2019

I just use cabal by itself while hacking on hedgehog (and most open source). If you want to proceed with nix, I suggest using callHackage to get the desired resourcet version like this, but I am not a nix expert.

@HuwCampbell
Copy link
Member

You can still hoist for resource handling.

test . hoist runResourceT $
  do_something

@endgame
Copy link
Contributor Author

endgame commented Mar 12, 2019

Thanks @HuwCampbell , I have pushed a version that relaxes the bound and uses hoist.

@jacobstanley
Copy link
Member

jacobstanley commented Mar 19, 2019

hedgehog-example needs that more restrictive bound since the example works with MonadBaseControl but does not work with MonadUnliftIO

@gwils
I am actually fairly worried about this, I don't think Property can implement MonadUnliftIO because it demands so much. There is only instances for ReaderT and IdentityT. So we stick to an old version of resourcet which might clash with projects trying to use the new one or we need our own resource handling, maybe another library which we can maintain.

On the other hand, if @HuwCampbell's solution works maybe that is the way.

@jacobstanley
Copy link
Member

As the first argument to Command is almost always an instance of MonadGen, I believe g is a better mnemonic than n. I can pursue this change so that all MonadGen references are g, or only the state-machine-test-related ones, or somewhere in between.

Yes, I agree, I think I'd prefer m in most places and I'm happy for you to change it to g where it's ambiguous.

I have also added to the haddock for Command. I found it hard to get my head around the way hedgehog's state machine testing support all fits together, and can create more documentation PRs if you like this sort of thing.

💯 Yeah that would be amazing! I find it a bit difficult myself tbh.

@gwils
Copy link
Contributor

gwils commented Mar 20, 2019

I confess I don't know much about resourcet or alternatives. Just that using runResource with hedgehog requires resourcet < 1.2 (although hedgehog itself is happy with resourcet 1.2.* for its internal usages)

I didn't know about Huw's hoist solution. It's probably worth investigating.

@endgame
Copy link
Contributor Author

endgame commented Mar 20, 2019

I have split the resourcet stuff into #255 as it seems like more thinking is required there. I have also expanded the haddocks (particularly around Symbolic and Concrete) and renamed n to g for MonadGen g within Hedgehog.Internal.State only.

At this point I want to move from "discussion" to "pushing for merge" on this PR, so please check the haddocks to see whether they make sense, etc.

@jacobstanley
Copy link
Member

jacobstanley commented Apr 2, 2019

Very nice, happy with this 💯

Thanks for doing that, sorry about the delay

@jacobstanley jacobstanley merged commit e6534d9 into hedgehogqa:master Apr 2, 2019
@endgame endgame deleted the command-var-name branch April 2, 2019 23:39
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.

4 participants