Implement Zipper Eq and Hash and add tests by gliptak · Pull Request #343 · functionaljava/functionaljava

gliptak · 2018-07-29T20:37:39Z

Signed-off-by: Gábor Lipták gliptak@gmail.com

codecov-io · 2018-07-29T20:42:20Z

Codecov Report

Merging #343 into master will decrease coverage by 0.54%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
- Coverage   41.49%   40.95%   -0.55%     
==========================================
  Files         176      176              
  Lines        9174     9189      +15     
  Branches      522      524       +2     
==========================================
- Hits         3807     3763      -44     
- Misses       5217     5264      +47     
- Partials      150      162      +12

Impacted Files	Coverage Δ
core/src/main/java/fj/Hash.java	`20.71% <0%> (-1.26%)`	⬇️
core/src/main/java/fj/Equal.java	`48.02% <40%> (-0.24%)`	⬇️
core/src/main/java/fj/data/Zipper.java	`31.57% <50%> (+31.57%)`	⬆️
core/src/main/java/fj/F7Functions.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/F3Functions.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/Digit.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/F6Functions.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/F8Functions.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/F5Functions.java	`0% <0%> (-100%)`	⬇️
core/src/main/java/fj/F4Functions.java	`0% <0%> (-100%)`	⬇️
... and 14 more

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 1849e1d...ae70d20. Read the comment docs.

jbgi · 2018-07-30T20:49:44Z

+      Zipper<A> x1 = a1;
+      Zipper<A> x2 = a2;
+
+      x1.move(0);


those two move(0) statements do nothing?

The zipper handed out might not be focus at start. Let me know if you think they are not needed.

but move is a pure function so you must use the result if you want to do that.
But probably the position of a zipper is important and I'd be in favor of just comparing lefts(), focus() and right(). This is also what the haskell implementations I have seen do.

thanks for the pointer. updated

jbgi · 2018-07-30T20:51:20Z

+        int r = 239;
+        Zipper<A> aas = as;
+
+        aas.move(0);


this does nothing?

The zipper handed out might not be focus at start. Let me know if you think they are not needed.

Same as for Equal, I'd say we just need to combine the hash of lefts(), focus() and right().

jbgi · 2018-07-31T06:08:59Z

+  public static <A> Equal<Zipper<A>> zipperEqual(final Equal<A> ea) {
+    Definition<A> eaDef = ea.def;
+    return equalDef((a1, a2) -> {
+      return Equal.equals0(Stream.class, a1.lefts(), a2.lefts(), () -> Equal.streamEqual(Equal.anyEqual())) &&


I think streamEqual should use Equal<A> not anyEqual. also It would be best to save the streamEqual in a local variable for reuse.
Also equals0need to be used.

partially updated. I'm not sure about equals0

jbgi · 2018-07-31T06:09:38Z

+        final int p = 419;
+        int r = 239;
+
+        r = p * r + streamHash(ha).hash(as.lefts());


also probably best to save streamHash(ha) in a local variable.

jbgi · 2018-08-01T07:59:47Z

@gliptak added a commit with remaining observations: aa295b9
Please squash it/fixup and I will merge.

Signed-off-by: Gábor Lipták <gliptak@gmail.com>

gliptak · 2018-08-01T22:15:42Z

thank you for the update. squashed.

jbgi reviewed Jul 30, 2018

View reviewed changes

jbgi reviewed Jul 31, 2018

View reviewed changes

Implement Zipper Eq and Hash and add tests

ae70d20

Signed-off-by: Gábor Lipták <gliptak@gmail.com>

jbgi merged commit 3777c1f into functionaljava:master Aug 2, 2018

jbgi added this to the 4.8 milestone Aug 18, 2018

Uh oh!

Conversation

gliptak commented Jul 29, 2018

Uh oh!

codecov-io commented Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbgi commented Aug 1, 2018

Uh oh!

gliptak commented Aug 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Jul 29, 2018 •

edited

Loading