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

#607: Add missing Show instances #928

Merged
merged 3 commits into from
Mar 16, 2016
Merged

Conversation

matt-martin
Copy link

Randomly picked up this issue as I saw it was marked as "low hanging fruit" and I wanted to get more familiar with cats (and associated FP concepts).

One thing of note: I had to add a show instance for Tuple2 to make this work. I can't claim to be intimately familiar with Miles' Kittens project (see: https://github.com/milessabin/kittens), but it seemed like defining Show instances for Tuples and case classes would be particularly tedious and possibly something that Kittens/Shapeless could make a lot easier.

Without further ado, here is my initial attempt to address issue #607 and add a Show instance for WriterT.

…ow could be automatically derived for Tuples and case classes (see: https://github.com/milessabin/kittens), but ignored that possibility for now and just wrote an instance for Tuple2 to get this to work.
@codecov-io
Copy link

Current coverage is 88.87%

Merging #928 into master will decrease coverage by -0.07% as of cabb413

@@            master    #928   diff @@
======================================
  Files          179     179       
  Stmts         2125    2130     +5
  Branches        42      42       
  Methods          0       0       
======================================
+ Hit           1890    1893     +3
  Partial          0       0       
- Missed         235     237     +2

Review entire Coverage Diff as of cabb413

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor

ceedubs commented Mar 13, 2016

Wonderful 👍. Thanks @matt-martin!

See #631 for some discussion regarding the tuple instance generation that you brought up.

@stew
Copy link
Contributor

stew commented Mar 16, 2016

👍

stew added a commit that referenced this pull request Mar 16, 2016
@stew stew merged commit d1b87a6 into typelevel:master Mar 16, 2016
@matt-martin
Copy link
Author

Thank you @stew and @ceedubs! Appreciate the reference to #631. Definitely interested in that discussion.

@@ -44,6 +44,8 @@ final case class WriterT[F[_], L, V](run: F[(L, V)]) {

def reset(implicit monoidL: Monoid[L], functorF: Functor[F]): WriterT[F, L, V] =
mapWritten(_ => monoidL.empty)

def show(implicit F: Show[F[(L, V)]]): String = F.show(run)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm we should be able to delete this line right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@adelbertc isn't the Show instance delegating to this method?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the Show instance is delegating to this method. I modeled this on the change that added a Show instance for OptionT (see: https://github.com/typelevel/cats/pull/600/files; and in particular:

def show(implicit F: Show[F[Option[A]]]): String = F.show(value)
). But I agree it is not strictly needed (assuming the Show instance is updated accordingly).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now, I read it as Show[(L, V)] - ignore me :-)

@ceedubs ceedubs mentioned this pull request May 14, 2016
19 tasks
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.

5 participants