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

Added minimal WriterT implementation. #542

Merged
merged 6 commits into from
Oct 4, 2015
Merged

Added minimal WriterT implementation. #542

merged 6 commits into from
Oct 4, 2015

Conversation

sungiant
Copy link
Contributor

No description provided.

package cats
package data

final case class WriterT[T[_], L, V] (run: T[(L, V)]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change T[_] to F[_]? If memory serves, everywhere else we are pretty much using F[_] (and G[_] if there are two type constructor params)

@ceedubs
Copy link
Contributor

ceedubs commented Sep 25, 2015

Thanks for submitting this! WriterT is a great addition to Cats.

I left some comments, but in general this looks great to me.

We should hook this up to some law-checking and tests. If you would be willing to do that on this PR, that would be great. Otherwise, I'm happy to work on that.

One other small stylistic note: in other parts of Cats we aren't putting a space in front of ( for defs and function calls. We probably should have a Scalastyle rule enforcing consistency for that, but in the meantime, would you be willing to match the convention?

Thanks again, @sungiant!

@sungiant
Copy link
Contributor Author

Hi @ceedubs, thanks for the feedback, I've made a few stylistic changes as per your suggestions.

I've not made the Kind-Projector change as I'm not very familiar with how to make the change, how would ({type WT[$] = WriterT[F, L, $]})#WT[A] be rewritten?

As for tests, I took a look, but I don't fully understand how the law-checking part works, it would be great if someone in the know could get the ball rolling.

@codecov-io
Copy link

Current coverage is 75.56%

Merging #542 into master will increase coverage by +0.14% as of 2d70740

@@            master    #542   diff @@
======================================
  Files          152     154     +2
  Stmts         2055    2091    +36
  Branches        68      68       
  Methods          0       0       
======================================
+ Hit           1550    1580    +30
  Partial          0       0       
- Missed         505     511     +6

Review entire Coverage Diff as of 2d70740

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor

ceedubs commented Sep 25, 2015

@sungiant thank you. You should be able to rewrite new Monad[({type WT[$] = WriterT[F, L, $]})#WT] as new Monad[WriterT[F, L, ?]] using kind-projector. I can add the law-checking (and maybe some other tests), but I probably won't be able to get around to it until mid-to-late next week.

@ceedubs
Copy link
Contributor

ceedubs commented Sep 27, 2015

@sungiant I've submitted a PR to your repo that uses kind projector and adds some law-checking and a couple tests.

Some things that we should probably do (but I think could wait until subsequent PRs):

  • Add a Writer alias for when F is Id
  • Add some more instances (Functor, Applicative, Bifunctor, Semigroup, Monoid, etc) along with law-checking for those instances
  • Add more ad-hoc tests

Add WriterT law-checking and tests
@ceedubs
Copy link
Contributor

ceedubs commented Sep 27, 2015

👍

Thanks @sungiant!

@non
Copy link
Contributor

non commented Oct 4, 2015

Looks great, thanks!

👍

non added a commit that referenced this pull request Oct 4, 2015
Added minimal WriterT implementation.
@non non merged commit c036249 into typelevel:master Oct 4, 2015
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