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

Implement traverseTap #3647

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Implement traverseTap #3647

merged 1 commit into from
Oct 30, 2020

Conversation

majk-p
Copy link
Contributor

@majk-p majk-p commented Oct 26, 2020

I'd like to introduce traverseTap. I found it would be useful for me when attempting to perform debug log executing an effect, but dropping the result.

I'd like to add some tests, could someone please advice the best place to add them?

kubukoz
kubukoz previously approved these changes Oct 26, 2020
core/src/main/scala/cats/Traverse.scala Show resolved Hide resolved
core/src/main/scala/cats/Traverse.scala Show resolved Hide resolved
LukaJCB
LukaJCB previously approved these changes Oct 26, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

I like it 👍

johnynek
johnynek previously approved these changes Oct 27, 2020
@johnynek
Copy link
Contributor

my personal opinion is that each function like this should be added to the laws, and the law is just a duplication of the implementation. That way, if someone overrides to make a more optimized version, we have a law that it behaves exactly as the direct implementation.

@majk-p majk-p dismissed stale reviews from johnynek and LukaJCB via c5b91c8 October 28, 2020 07:33
@majk-p
Copy link
Contributor Author

majk-p commented Oct 28, 2020

my personal opinion is that each function like this should be added to the laws, and the law is just a duplication of the implementation. That way, if someone overrides to make a more optimized version, we have a law that it behaves exactly as the direct implementation.

That make sense. Implemented in https://github.com/typelevel/cats/pull/3647/files#diff-5ee4897e7202dfd0b4fd61f8afac00bbc7a9b28dc9309b3006f3ffc12b2cd36eR55. Couldn't find a better name for this law.

@johnynek
Copy link
Contributor

this is great! Unfortunately, we also need to add a line here:

"traverse ref zipWithIndex" -> forAll(laws.zipWithIndexRef[A, C] _)

to actually call this code when the tests are run.

@majk-p
Copy link
Contributor Author

majk-p commented Oct 29, 2020

Thank you for pointing this out @johnynek. Sorry I've missed that, I'm not yet familiar enough with the project.
Just fixed that.

johnynek
johnynek previously approved these changes Oct 29, 2020
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks! This looks useful for logging.

@majk-p
Copy link
Contributor Author

majk-p commented Oct 29, 2020

Had to make one more push to avoid breaking binary compatibility in laws

@codecov-io
Copy link

Codecov Report

Merging #3647 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3647   +/-   ##
=======================================
  Coverage   90.24%   90.25%           
=======================================
  Files         391      391           
  Lines        8855     8863    +8     
  Branches      256      248    -8     
=======================================
+ Hits         7991     7999    +8     
  Misses        864      864           

@johnynek johnynek merged commit f3f0dd3 into typelevel:master Oct 30, 2020
@larsrh larsrh modified the milestones: 3.0, 2.3 Oct 31, 2020
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.

6 participants