zipper: issue with latest test case from sync #902

Activity

1. SleeplessByte

   added

   sync 🔄Sync an exercise with the latest version of the problem-specSync an exercise with the latest version of the problem-spec

   good first issue

   on Dec 24, 2020
2. junedev

   added

   x:action/syncSync content with its latest versionSync content with its latest version

   x:knowledge/elementaryLittle Exercism knowledge requiredLittle Exercism knowledge required

   x:module/practice-exerciseWork on Practice ExercisesWork on Practice Exercises

   x:size/smallSmall amount of workSmall amount of work

   x:type/contentWork on content (e.g. exercises, concepts)Work on content (e.g. exercises, concepts)

   on Sep 6, 2021
3. sachinmk27 commented on Sep 17, 2021

   

   sachinmk27

   on Sep 17, 2021

   Contributor

   More actions

   I'd like to work on this
4. sachinmk27

   added a commit that references this issue on Sep 17, 2021

   Add test case for practice/zipper. Fix exercism#902

   This issue will close once commit 1e0108c is merged into the 'main' branch.

   1e0108c
5. junedev

   added

   x:status/claimedSomeone is working on this issueSomeone is working on this issue

   on Sep 18, 2021
6. junedev

   closed this as completedin #1354on Sep 20, 2021
7. junedev

   added a commit that references this issue on Sep 20, 2021

   Add test case for practice/zipper. Fix #902 (#1354)

   This issue will close once commit 200b1c8 is merged into the 'main' branch.

   200b1c8
8. joshgoebel commented on Mar 4, 2022

   

   joshgoebel

   on Mar 4, 2022

   Last edited by joshgoebel

   Contributor

   More actions

   @SleeplessByte I don't understand this test at all... It seems wholly at odds with the problem specificiation:

   • prev (move the focus to the previous child of the same parent, returns a new zipper)
   • next (move the focus to the next child of the same parent, returns a new zipper)
   • up (move the focus to the parent, returns a new zipper)
   • set_value (set the value of the focus node, returns a new zipper)

   All mutation/navigation returns a new zipper, this directly contradicts the most recent test of "same zipper".

   Should I open a new issue?
9. joshgoebel commented on Mar 4, 2022

   

   joshgoebel

   on Mar 4, 2022

   Last edited by joshgoebel

   Contributor

   More actions

   To me I see 3 levels of abstraction here...

   1. the underlying node data in memory
   2. the JavaScript reference to a node (variable, object key, etc)
   3. the Zipper instance (which holds unto a node reference)

   I might have two zippers a and b that hold the same reference (2) to the same data(1) - because they are the same path (even if arrived at my different routes), but they are (in my implementation) different pointer instances (3)... I'm confused as to why this should be a requirement or even exactly what it means for it to be a requirement.

   The directions say nothing about this requirement.

   Questions:

   • It seems that what matters here should be 1/2 (pointer equality) not 3 (whether we have 1 or 2 discrete pointers), no?
   • And of course this stops being true if one calls a set anywhere along the route, correct?
   • How does this test even pass given the code for right - it literally calls new Zipper() always...
10. joshgoebel commented on Mar 4, 2022

    

    joshgoebel

    on Mar 4, 2022

    Last edited by joshgoebel

    Contributor

    More actions

    This also seems a direct contradiction of the immediate prior test:

      test('up returns a new Zipper', () => {
        const up = zipper.right().up();
        expect(zipper).not.toBe(up);
      });

    These are the same path (root), so why are they not the "same" zipper?
11. joshgoebel commented on Mar 4, 2022

    

    joshgoebel

    on Mar 4, 2022

    Last edited by joshgoebel

    Contributor

    More actions

    Wow... I still think there is a lot to be confused about here (and I'm conflating toBe and toEqual above), but I don't think the test names help add much clarity in this regard... "same zipper" in what sense? I'd personally word it more like "different paths lead to same node" (which is actually what is truly being compared, not zipper instances).

    ...but I did get the last test to pass easily by changing:

      value = () => this.node.value
    

    (which seems to trip up equality since I assume there are now two different value functions)

    to

        value() {
            return this.node.value
        }
    

    Are we purposely intending to penalize students for writing code in this fashion?

    ---

    Proposal:

    test('should access same node via different paths', () => {
        const z1 = zipper.left().up().right();
        const z2 = zipper.right();
        expect(z1.node).toBe(z2.node);
    });```
12. junedev

    reopened this on Mar 16, 2022
13. junedev

    added

    discussion 💬

    and removed

    good first issue

    sync 🔄Sync an exercise with the latest version of the problem-specSync an exercise with the latest version of the problem-spec

    x:action/syncSync content with its latest versionSync content with its latest version

    x:knowledge/elementaryLittle Exercism knowledge requiredLittle Exercism knowledge required

    x:module/practice-exerciseWork on Practice ExercisesWork on Practice Exercises

    x:status/claimedSomeone is working on this issueSomeone is working on this issue

    x:type/contentWork on content (e.g. exercises, concepts)Work on content (e.g. exercises, concepts)

    x:size/smallSmall amount of workSmall amount of work

    on Mar 16, 2022
14. junedev

    changed the title [-]zipper: sync with canonical data[/-] [+]zipper: issue with latest test case from sync[/+] on Mar 16, 2022
15. junedev commented on Mar 16, 2022

    

    junedev

    on Mar 16, 2022

    Member

    More actions

    @joshgoebel I re-opened this and adjusted the title and labels, then you don't have to copy your thoughts into a new issue.
    I put "discussion" for now until a maintainer had time to take a closer look and decide how to proceed.