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

Add an explanation for the missing Applicative instances for monad transformers #2277

Merged
merged 3 commits into from
Dec 19, 2018
Merged

Add an explanation for the missing Applicative instances for monad transformers #2277

merged 3 commits into from
Dec 19, 2018

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Jun 3, 2018

Closes #2272.

Just a few lines to start with, hope you have some feedback on what else I can mention/what can be reworded :)

@codecov-io
Copy link

codecov-io commented Jun 3, 2018

Codecov Report

Merging #2277 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2277   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files         364      364           
  Lines        6707     6707           
  Branches      285      301   +16     
=======================================
  Hits         6380     6380           
  Misses        327      327

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b329cf1...3b8dd0e. Read the comment docs.

kailuowang
kailuowang previously approved these changes Jul 31, 2018
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks!

@kailuowang kailuowang requested a review from LukaJCB August 3, 2018 20:20
@LukaJCB
Copy link
Member

LukaJCB commented Aug 3, 2018

I like it, but can we extract a bit from the issue you linked, I feel like just linking to an issue will cause people to have to wade through a bunch of discussion instead of getting to the actual point. You know what I mean? 🤔

@kubukoz
Copy link
Member Author

kubukoz commented Aug 6, 2018

Sure, I'll look for something that can be there instead of the link.

@kubukoz
Copy link
Member Author

kubukoz commented Aug 20, 2018

Can we just drop the link? The potential inconsistency is already mentioned. WDYT?

@LukaJCB
Copy link
Member

LukaJCB commented Aug 21, 2018

I think that might be more confusing, then it'd just say "This is inconsistent" without naming a good reason.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 21, 2018

Maybe add the counter example noted here? #1106 (comment)

Of course would need porting to EitherT or OptionT instead. :)

Copy link
Contributor

@easel easel left a comment

Choose a reason for hiding this comment

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

LGTM. I agree it would be better to extract the content from the linked ticket.

@kubukoz
Copy link
Member Author

kubukoz commented Sep 25, 2018

@easel I haven't had much time to work on this recently, but I'll surely extract something before I consider this good to merge :P

@kubukoz
Copy link
Member Author

kubukoz commented Dec 18, 2018

@LukaJCB as part of this week's goal to finish up all open OSS PRs, I updated this one - please take a look :)

@kubukoz
Copy link
Member Author

kubukoz commented Dec 19, 2018

@kailuowang FYI

@kailuowang kailuowang merged commit d117df8 into typelevel:master Dec 19, 2018
@kailuowang
Copy link
Contributor

Thanks very much @kubukoz !

@kailuowang kailuowang added this to the 1.6 milestone Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants