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

Jump to bottom

More taps #16

Merged
merged 5 commits into from Jul 12, 2018
Merged

More taps #16

merged 5 commits into from Jul 12, 2018

Conversation

albertpastrana
Copy link
Owner

This PR adds some useful functions we have been using as implicits in other projects.

@nathankleyn it would be great if you could take a look at it

@albertpastrana
Adds flags to speed up compilation
fa41d8f 
Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@albertpastrana
Adds tapFail
a096e75 
Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@albertpastrana
Makes isOk a val
6fbce5e 
Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@albertpastrana
Adds toResult to Option, Either & Try
e0e59c4 
Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
@codecov-io
Copy link

codecov-io commented Jul 11, 2018 •
edited

Codecov Report

Merging #16 into master will increase coverage by 0.45%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   90.65%   91.11%   +0.45%     
==========================================
  Files          16       16              
  Lines         214      225      +11     
  Branches        1        1              
==========================================
+ Hits          194      205      +11     
  Misses         20       20
Impacted Files Coverage Δ
...n/scala/uscala/concurrent/result/AsyncResult.scala 100% <100%> (ø) ⬆️
result/src/main/scala/uscala/result/Result.scala 100% <100%> (ø) ⬆️

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 335aed5...4c4cf88. Read the comment docs.

@coveralls
Copy link

coveralls commented Jul 11, 2018 •
edited

Coverage Status

Coverage decreased (-1.5%) to 98.522% when pulling f2a7819 on more-taps into 335aed5 on master.

@coveralls
Copy link

coveralls commented Jul 11, 2018 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 4c4cf88 on more-taps into 335aed5 on master.

@coveralls
Copy link

coveralls commented Jul 11, 2018 •
edited

Coverage Status

Coverage decreased (-1.5%) to 98.522% when pulling f2a7819 on more-taps into 335aed5 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) to 98.522% when pulling f2a7819 on more-taps into 335aed5 on master.

nathankleyn
nathankleyn requested changes Jul 12, 2018
View reviewed changes
Copy link
Collaborator

@nathankleyn nathankleyn left a comment

Choose a reason for hiding this comment

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

Couple of missing specs, but implementation looks great and these are awesome additions! 👍

result-async/src/test/scala/uscala/concurrent/result/AsyncResultSpec.scala
@@ -137,6 +137,64 @@ class AsyncResultSpec(implicit ee: ExecutionEnv) extends Specification with Scal
}
}

"tap" >> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we're missing tests for tapOk - even though it's an alias, we should probably test it anyway (for coverage and catching accidents).

result/src/test/scala/uscala/result/ResultSpec.scala Outdated
@@ -123,6 +123,27 @@ class ResultSpec extends Specification with ScalaCheck {
}
}

"tapFail" >> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, probably best to test tapOk even if it is just an alias.

result/src/test/scala/uscala/result/ResultSpec.scala
}
}
}

"sequence for options" >> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're missing a spec for fromSomething?

Copy link
Owner Author

Choose a reason for hiding this comment

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

sorry, I pushed this one by mistake, I'll remove the function

@nathankleyn nathankleyn self-assigned this Jul 12, 2018
@albertpastrana
Adds tap, tapFail, bitap and flatten to AsyncResult
4c4cf88 
Signed-off-by: Albert Pastrana <albert.pastrana@gmail.com>
nathankleyn
nathankleyn approved these changes Jul 12, 2018
View reviewed changes
Copy link
Collaborator

@nathankleyn nathankleyn left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nathankleyn nathankleyn merged commit 04e2790 into master Jul 12, 2018
@nathankleyn nathankleyn deleted the more-taps branch July 12, 2018 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathankleyn nathankleyn nathankleyn approved these changes

Assignees

@nathankleyn nathankleyn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@albertpastrana @codecov-io @coveralls @nathankleyn