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.

Sign up for GitHub

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

Jump to bottom

MonadFail migration #48

Closed
wants to merge 4 commits into from
Closed

MonadFail migration #48

wants to merge 4 commits into from

Conversation

ejconlon
Copy link

@ejconlon ejconlon commented May 8, 2020

Tests pass with Stackage LTS-15.11.

@tdammers
Copy link
Owner

Hmm, I'm not 100% sure I like it like this.

Getting things to compile cleanly on 8.8 is of course necessary, but I think that rather than make fail work, we should get rid of it - it's kind of an ad-hoc solution to the problem of dealing with failures, and I'd much rather resort to a more "proper" solution, probably something along the lines of a more descriptive error type and ExceptT.

I'm particularly unhappy with extending runGinger, which is supposed to be (technically and morally) pure to return an arbitrary m.

I'll do some thinking on this and get back to you.

@tdammers tdammers mentioned this pull request May 13, 2020
Closed
@tdammers
Copy link
Owner

On this note: #49

@ejconlon
Copy link
Author

Ok, thanks for looking!

tdammers added a commit that referenced this pull request May 14, 2020
@tdammers
MonadFail migration
4258d0d 
Rather than peppering the codebase with MonadFail constraints, I'm
taking this opportunity to get rid of those "dirty" failures, and
use the existing (!) MonadError instance already built into the Run
monad.

See #48, #49.
@ejconlon ejconlon closed this May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdammers tdammers tdammers left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ejconlon @tdammers