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 some more tests for HMap. #8

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Add some more tests for HMap. #8

merged 3 commits into from
Aug 30, 2017

Conversation

non
Copy link
Collaborator

@non non commented Aug 29, 2017

At this point I think we're covering all the methods, certainly all
the ones we are leaning on in the other code.

At this point I think we're covering all the methods, certainly all
the ones we are leaning on in the other code.
@codecov-io
Copy link

codecov-io commented Aug 29, 2017

Codecov Report

Merging #8 into master will increase coverage by 1.68%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage    84.4%   86.09%   +1.68%     
==========================================
  Files          12       12              
  Lines         186      187       +1     
  Branches       13       13              
==========================================
+ Hits          157      161       +4     
+ Misses         29       26       -3
Impacted Files Coverage Δ
core/src/main/scala/com/stripe/dagon/HMap.scala 90.47% <100%> (+15.47%) ⬆️

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 38f21d7...0de5a79. Read the comment docs.

Copy link
Collaborator

@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.

one small concern about making a private method public.

@@ -80,6 +83,6 @@ object HMap {
def empty[K[_], V[_]]: HMap[K, V] =
from[K, V](Map.empty[K[_], V[_]])

private def from[K[_], V[_]](m: Map[K[_], V[_]]): HMap[K, V] =
def from[K[_], V[_]](m: Map[K[_], V[_]]): HMap[K, V] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have to make this public? This is unsafe and defeating the main use of the code (we know that the inner types match for the keys).

It looks like you are just using it to generate HMaps, but I think we can do that by doing .update with the right way, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make it private[dagon] -- I'm just using it in tests.

@johnynek johnynek merged commit e44c404 into master Aug 30, 2017
@non non deleted the add-hmap-tests branch August 30, 2017 14:58
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