WIP zipper: major change, adding immutability tests and node insertion tests

[cglacet]

The main goal is to be able to run tests without using an intermediate tree structure.

This follows issue exercism/python#1497.

Now the test consists in creating a tree starting from an empty zipper
using the the atomic operations left, right, up and insert/modify/read
node values. (Each operation is tested independently.)

In the old version there were no immutability check. Immutability will
now be an important part of students goals.

[cglacet]

I've implemented this in python (solution, skeleton and test files). Let me know if you have questions about this.

[petertseng]

Meaning of affectTo and applyOn

I think to help those who would read this file, the file's comments should include the meaning of the affectTo and applyOn keys of each operation. It is not otherwise immediately obvious. It's possible to guess, but we should save future readers of this file from having to do the guesswork.

Descriptions

I have a preference that the descriptions don't start with "Test that X" and should rather just say "X" simply it's implied from their presence in this file that the are all tests. However, I can't point to a policy that says this is what should happen. Thus, I cannot require that this happen.

Property

I am suspicious of the property because they seem too specific (some of them simply restate the description, such as testInsertNonMutating, I would have expected insert), but I'd have to write a verifier to be certain of this fact.

We would normally expect the property to be the function under test, though for tests that are structured as strings of operations there is no specific function (https://github.com/exercism/problem-specifications/blob/master/exercises/circular-buffer/canonical-data.json , https://github.com/exercism/problem-specifications/blob/master/exercises/react/canonical-data.json)

The rule that must be true for test generators to work properly is that if two tests have the same property, their inputs and outputs must be interpreted the same way by the generator.

I will make a more specific recommendation after I've written a verifier. I do not have a specific recommendation right now. I promise that I will write one, and I want to do it in the coming week, but I can't make a more specific promise about when it will happen

[cglacet]

I think to help those who would read this file, the file's comments should include the meaning of the affectTo and applyOn keys of each operation. It is not otherwise immediately obvious. It's possible to guess, but we should save future readers of this file from having to do the guesswork.

Sure!

I have a preference that the descriptions don't start with "Test that X" and should rather just say "X" simply it's implied from their presence in this file that the are all tests. However, I can't point to a policy that says this is what should happen. Thus, I cannot require that this happen.

Ok, I don't have any opinion on that, I'll trust you.

I am suspicious of the property because they seem too specific (some of them simply restate the description, such as testInsertNonMutating, I would have expected insert), but I'd have to write a verifier to be certain of this fact.

I'm not sure to get what you mean by that. I have two tests for insertion, one that tests if insertions work properly, and the other check if its not mutating the input object.

We would normally expect the property to be the function under test

Ah ok, I read that but I wasn't too sure what that means and the original file used to name properties the way I did it here. Let me know what they should look like.

The rule that must be true for test generators to work properly is that if two tests have the same property, their inputs and outputs must be interpreted the same way by the generator.

What do you call output and input?

I will make a more specific recommendation after I've written a verifier. I do not have a specific recommendation right now. I promise that I will write one, and I want to do it in the coming week, but I can't make a more specific promise about when it will happen

Ok, thanks.

[petertseng]

The rule that must be true for test generators to work properly is that if two tests have the same property, their inputs and outputs must be interpreted the same way by the generator.

What do you call output and input?

output is expected and input is input. The submitted file is doing the right thing already

[petertseng]

If you don't mind, I would ask about a few details of the proposed zipper interface to make sure reviewers understand

A major feature of the proposed interface is the ability to navigate to a spot at which a node is to be inserted. For example, from any arbitrary leaf L, move left, then insert a value there. From this a few interesting questions result:

• From an arbitrary leaf, I move left (to a space where I can insert a node), then left again. What should happen?
  • (Only if applicable based on answer to previous question): From an arbitrary leaf, I move left, then left, then insert. What should happen? (If something is inserted, does the parent have a value?)
• Should I understand insert to only work where there is no existing node, and set_value to only work when there is an existing node? Is it an error to set_value on a space where a node doesn't exist, and/or is it an error to insert where a node already exists?
• Will set_left and set_right still exist in the proposed interface? The usefulness of these two operations is to replace an entire subtree, which set_value cannot do alone.
  • (If they will not exist) Is it acceptable to drop them? What is the proposed alternative for someone who wishes to achieve the goal of replacing an entire subtree?
  • (If they will exist) Should they be tested?

[cglacet]

From an arbitrary leaf, I move left (to a space where I can insert a node), then left again. What should happen?

It should raise an error. I though I added this in the tests, but maybe I changed this after the push. I'll have a look at it as soon as I can free a bit of time (sadly a few days).

Should I understand insert to only work where there is no existing node

Exactly.

and set_value to only work when there is an existing node?

Yes, this function only modifies the value of a node. Whereas insert add a node in some empty space.

Is it an error to set_value on a space where a node doesn't exist,

It definitely should yes.

and/or is it an error to insert where a node already exists?

I'm not really sure about that, we could imagine that insert simply replaces the existing node if there is one. Note that this node can be a tree, this would thus remove the whole tree and replace it by a new node.

Will set_left and set_right still exist in the proposed interface?

If you have a look at my python solution you'll see that I added two macros that do exactly what set_left and set_right used to do. It simply uses the existing methods to first move left (or right) then insert a node, and move back up. This could also be tested but I didn't added it as it's only a shortened version of the sequence: "set_left(x) = left insert(x) up".

We could also have the ability to insert more than a node (using its value), we could have insert and insert_tree and even insert_zipper they would all work the same way and simply take as argument objects of different types (a value, a tree or a zipper). I think we may also only have insert to only take zippers as input, this would make the exercise cleaner. I'm not sure how to describe that in the test description though.

Is it acceptable to drop them? (set_left and set_right still)

I really think it can be dropped.

Should they be tested?

If they exist then yes.

[petertseng]

Although 15 days ago I said I wanted to write a verifier in the next week, I'm now overdue by over a week. I have no excuse for that. I apologise, but I realise the apology is not very useful because of not providing definitive answers. It is not providing definitive answers because I'd need to think more about what I want out of a zipper. I think I will understand more once I have written it.

and/or is it an error to insert where a node already exists?

I'm not really sure about that, we could imagine that insert simply replaces the existing node if there is one. Note that this node can be a tree, this would thus remove the whole tree and replace it by a new node.

I am starting to think the zipper should in fact only take in a tree in insert. If it takes only a single value, and it is understood that insert will create a leaf with that value, it seems that the API will have one of the following disadvantages:

• will have set_left and set_right but their types (they take trees) will be inconsistent with insert (takes a value), and that inconsistency seems strange.
• won't have set_left or set_right, but it seems too cumbersome to create large trees with repeated moves and inserts rather than providing the tree at once (imagine a situation where the tree is already given to us by some means)

I think we may also only have insert to only take zippers as input, this would make the exercise cleaner. I'm not sure how to describe that in the test description though.

Yes, I am also not sure how that would work. I want to learn more about taking zipper as input to insert. Consider a zipper z and then a zipper z' that results only from moving from z, not inserting at any point. Is there a difference between insert(z) and insert(z')?

• If there is no difference, that means the focus of the zipper does not affect the result, in which case providing the focus to be unnecessary information. The API would be clearer (takes in only what it needs) if it were only to take a tree.
• If there is a difference, I suppose I'm to understand that the focus changes to that of the inserted zipper. That could be interesting, though hard to test that everything is working right. It seems to make more sense to keep the operations simple so that the tests can be simple, but I could be convinced otherwise if it turns out that the tests for this operation are in fact simple.

[petertseng]

OK, I've written the verifier, now I need to write the suggestions for this PR based on the output of the verifier. The verifier is as follows

require 'json'
require_relative '../../verify'

OPPOSITE = {
  left: :right,
  right: :left,
}.freeze

module Zipper
  def self.empty
    {tree: nil, context: [].freeze}.freeze
  end

  refine Hash do
    def down(direction)
      tree = self[:tree]

      {
        tree: tree[direction],
        context: ([
          [direction, tree[:value], tree[OPPOSITE.fetch(direction)].freeze].freeze
        ] + self[:context]).freeze,
      }.freeze
    end

    def up
      return nil if self[:context].empty?
      direction, value, other = self[:context].first
      new_tree = {
        value: value,
        direction => self[:tree].freeze,
        OPPOSITE.fetch(direction) => other.freeze,
      }.freeze
      {tree: new_tree, context: self[:context][1..-1].freeze}.freeze
    end

    def root
      self[:context].size.times.reduce(self) { |z, _| z.up }.freeze
    end
  end
end

using Zipper

def leaf(v)
  {value: v, left: nil, right: nil}.freeze
end

def verify_ignoring_property(cases, *args, &block)
  cases.chunk { |c| c['property'] }.each { |prop, cs|
    verify(cs, property: prop, &block)
  }
end

json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))

verify_ignoring_property(json['cases']) { |i, _|
  final_names = i['operations'].reduce({i['initialZipper'].fetch('name') => Zipper.empty}.freeze) { |names, op|
    names.merge(
      op['affectTo'] => case op['operation']
      when 'insert'
        zipper = names[op['applyOn']]
        raise "No, the zipper is not empty, it is #{zipper}" if zipper[:tree]
        zipper.merge(tree: leaf(op['item'])).freeze
      when 'value'
        names[op['applyOn']][:tree][:value]
      when 'left', 'right'
        names[op['applyOn']].down(op['operation'].to_sym)
      when 'up', 'top'
        # ??? Is top just an alias for up?
        names[op['applyOn']].up or raise "can't go up"
      when 'root'
        names[op['applyOn']].root
      when 'multiply'
        names[op['applyOn'][0]] * op['applyOn'][1]
      when 'setValue'
        # NEEDS TO BE DOCUMENTED
        v = op['item'].is_a?(String) ? names.fetch(op['item']) : op['item']
        names[op['applyOn']].merge(tree: zipper[:tree].merge(value: v)).freeze
      when 'createEmptyZipper'
        Zipper.empty
      when 'insertZipper'
        # THIS IMPLEMENTATION IS OBVIOUSLY BOGUS,
        # because the operation is not well-tested in the tests.
        names[op['item']].root
      when 'floorDivide'
        names[op['applyOn'][0]] / names[op['applyOn'][1]]
      else raise "unknown op #{op}"
      end
    )
  }

  # THIS SHOULD NOT BE HERE; NEED TO MKE EXPLICIT WHAT VALUE IS BEING COMPARED
  {'type' => 'int', 'value' => final_names.fetch('actual')}
}

[petertseng]

I wonder if it's the case that languages where immutability is the default would want different tests than those languages where mutability is the default. All the "test that operation X doesn't mutate its operand" are not useful for those languages.

If this is true, I wonder how we deal with it and express it in this file. One idea may be to leave the tests in the JSON file but mark them in such a way; that way a generator for a language track where immutability is the default can know to ignore them.

We can reserve the discussion of implementation for when it becomes certain that such a measure is necessary.

[petertseng]

About explicitly giving a name to each intermediate value in computation:

I hope that does not make the tests any less elegant for languages focused on immutability and functional composition. Consider a test that originally looked like that in https://github.com/exercism/haskell/blob/master/exercises/zipper/test/Tests.hs :

      (left . fromJust . left . fromTree) t1
      `shouldBe` Nothing

If we had to give names to each intermediate value, we would not be able to reuse the names if they have different types. As a very naive translation, if we try:

      -- (left . fromJust . left . fromTree) t1
      let z = fromTree t1 in -- z is of type Zipper a
      let z = left z in -- does NOT compile! left's type is `Zipper a -> Maybe (Zipper a)`
      let z = fromJust z in
      let z = left z in
      z `shouldBe` Nothing

So that doesn't work. And we have a high chance of running into this issue because many names are re-used in this JSON file. Of course, it would be possible if we used different names for everything:

      -- (left . fromJust . left . fromTree) t1
      let z1 = fromTree t1 in
      let z2 = left z1 in
      let z3 = fromJust z2 in
      let z4 = left z3 in
      z4 `shouldBe` Nothing

But it really seems like these intermediate names are not adding a whole lot. So we have to see how to deal with that.

Most of the time, we are taking a single zipper, applying some sequence of operations (possibly composed) to that zipper, and at times it's important to give a name to a particular intermediate result so that we can check something about that intermediate result.

When dealing with this, we want to make the common case easy.

[petertseng]

Most of the time, we are taking a single zipper, applying some sequence of operations (possibly composed) to that zipper, and at times it's important to give a name to a particular intermediate result so that we can check something about that intermediate result.

When dealing with this, we want to make the common case easy.

So given the goal that we want to make the common case easy, I want to propose how I might express two of the submitted cases, to validate that a proposal even makes sense

      {
         "description":"Left move and an empty spot insertion.",
         "property":"left",
         "input":{
            "initialZipper":{
               "name":"zipper"
            },
            "operations":[
               {
                  "operation":"insert",
                  "affectTo":"zipper",
                  "applyOn":"zipper",
                  "item":1
               },
               {
                  "operation":"left",
                  "affectTo":"zipper",
                  "applyOn":"zipper"
               },
               {
                  "operation":"insert",
                  "affectTo":"zipper",
                  "applyOn":"zipper",
                  "item":2
               },
               {
                  "operation":"value",
                  "affectTo":"actual",
                  "applyOn":"zipper"
               }
            ]
         },
         "expected":{
            "type":"int",
            "value":2
         }
      },
      {
         "description":"Left move do not mutate initialZipper.",
         "property":"left",
         "input":{
            "initialZipper":{
               "name":"zipper"
            },
            "operations":[
               {
                  "operation":"insert",
                  "affectTo":"zipper",
                  "applyOn":"zipper",
                  "item":1
               },
               {
                  "operation":"left",
                  "affectTo":"tmp",
                  "applyOn":"zipper"
               },
               {
                  "operation":"value",
                  "affectTo":"actual",
                  "applyOn":"zipper"
               }
            ]
         },
         "expected":{
            "type":"int",
            "value":1
         }
      },

For those, I think I would say:

      {
         "description":"Left move and an empty spot insertion.",
         "property":"propertyToBeDecidedLater",
         "input":{
            "operations":[
               {
                  "operation":"insert",
                  "item":1
               },
               {
                  "operation":"left"
               },
               {
                  "operation":"insert",
                  "item":2
               },
               {
                  "operation":"value"
               }
            ]
         },
         "expected": 2
      },
      {
         "description":"Left move do not mutate zipper.",
         "property":"propertyToBeDecidedLater",
         "input":{
            "operations":[
               {
                  "operation":"insert",
                  "item":1
               },
               {
                  "operation":"save",
                  "name":"tmp"
               },
               {
                  "operation":"left"
               },
               {
                  "operation":"value",
                  "applyOn":"tmp"
               }
            ]
         },
         "expected": 1
      },

Here, the first operation is always implied to act on an empty zipper, and all subsequent operation are implied to act on the result of the previous operation, unless their applyOn states otherwise.

How I envision these working for languages that generally feature immutability and functional composition:

(value . insert 2 . left . insert 1) empty `shouldBe` 2

-- they might even omit this test altogether if they're sure it's pointless to test
let tmp = insert 1 empty in
let _ = left tmp in
value tmp `shouldBe` 1

How I envision these working for languages that generally feature mutability and objects receiving messages

expect(empty_zipper.insert(1).left.insert(2).value).to be == 2

tmp = empty_zipper.insert(1)
tmp.left
expect(tmp.value).to be == 1

I think this change to have fewer intermediate names will both improve the readability of the file for humans and also make translating the tests into concerned languages easier.

[petertseng]

The idea that we should be able to move the zipper into an empty space is pretty crucial. It seems we've missed this throughout the entire history of having this exercise on Exercism ever since exercism/exercism#964 . As just one example, see from http://learnyouahaskell.com/zippers that this should obviously be a thing we should be able to do.

[petertseng]

Did I list all tracks with this exercise? I didn't find where I did that yet. As of July:

csharp/exercises/zipper
elixir/exercises/zipper
erlang/exercises/zipper
fsharp/exercises/zipper
haskell/exercises/zipper
java/exercises/zipper
javascript/exercises/zipper
ocaml/exercises/zipper
python/exercises/zipper
ruby/exercises/zipper
scala/exercises/zipper

[cglacet]

I am starting to think the zipper should in fact only take in a tree in insert. If it takes only a single value, and it is understood that insert will create a leaf with that value, it seems that the API will have one of the following disadvantages:

• will have set_left and set_right but their types (they take trees) will be inconsistent with insert (takes a value), and that inconsistency seems strange.

I the exampler I linked, set_left and set_right just do the following actions: move left (/right), insert, move up. Both methods take a value as input, just as insert.

• won't have set_left or set_right, but it seems too cumbersome to create large trees with repeated moves and inserts rather than providing the tree at once (imagine a situation where the tree is already given to us by some means).

Yes, being able to insert a zipper at some location seems to be important. But in the other hand, if you don't have a way to insert values, then you'll have to provide a way to create a node (a single node zipper) with a given value. I feel like the insert method is more natural as you would just have to write something like insert_value(x) instead of insert(Node(x)).

Yes, I am also not sure how that would work. I want to learn more about taking zipper as input to insert. Consider a zipper z and then a zipper z' that results only from moving from z, not inserting at any point. Is there a difference between insert(z) and insert(z')?

They should both output the same tree.

• If there is no difference, that means the focus of the zipper does not affect the result, in which case providing the focus to be unnecessary information. The API would be clearer (takes in only what it needs) if it were only to take a tree.

Yes the focus doesn't make any difference, but I think it's clearer to insert zippers instead of trees. I think that's not unusual to have operations on an object that only consider a subset of the input characteristics. For example, when you write 12 mod 2 the mod function only look at the last bit of 12. In this case we would either have to write insert(z) or insert(z.tree), the first being shorter I tend to prefer it (note that the insert method will have to retrieve the tree anyway, the question is, who should retrieve it).

---

I wonder if it's the case that languages where immutability is the default would want different tests than those languages where mutability is the default. All the "test that operation X doesn't mutate its operand" are not useful for those languages.

If there is no way to mutate objects, for sure this test is useless, is this a problem to have tests that are always valid?

One idea may be to leave the tests in the JSON file but mark them in such a way; that way a generator for a language track where immutability is the default can know to ignore them.

That seems right to me.

---

And we have a high chance of running into this issue because many names are re-used in this JSON file

Shouldn't each test be encapsulated? Temporary names are just there because I had no idea how to write the tests in the JSON file, but when I wrote:

   {
      "operation":"left",
      "affectTo":"tmp",
      "applyOn":"zipper"
   },
   {
      "operation":"up",
      "affectTo":"tmp",
      "applyOn":"tmp"
   },
   {
      "operation":"value",
      "affectTo":"actual",
      "applyOn":"tmp"
   }

This translates to something like (in python):

actual = zipper.left.up.value

---

So given the goal that we want to make the common case easy, I want to propose how I might express two of the submitted cases, to validate that a proposal even makes sense

{
"operation":"insert",
"affectTo":"zipper",
"applyOn":"zipper",
"item":1
},
{
"operation":"left",
"affectTo":"zipper",
"applyOn":"zipper"
},

Which you want to replace with:

> ```
 {
     "operation":"insert",
     "item":1
},
{
     "operation":"left",
},

Here, the first operation is always implied to act on an empty zipper, and all subsequent operation are implied to act on the result of the previous operation, unless their applyOn states otherwise.

I have no idea how this JSON file will be used to generate tests so I'll trust you on this one, I just tried to read an existing JSON test specification an replicate what I found in it. If you can specify such rules formally then sure, it's easier to read (but is it really important that this JSON file is readable by a human?). Sorry if I'm confused about things that are obvious to you 😛

---

The idea that we should be able to move the zipper into an empty space is pretty crucial. It seems we've missed this throughout the entire history of having this exercise on Exercism ever since exercism/exercism#964 . As just one example, see from http://learnyouahaskell.com/zippers that this should obviously be a thing we should be able to do.

I'll have a look at this, for now I don't understand what you mean by "moving the zipper into an empty space".

[petertseng]

set_left and set_right just do the following actions: move left (/right), insert, move up. Both methods take a value as input, just as insert.

I think that is undesirable to take only values, but I think you have already agreed with that (by saying it is important to be able to insert a zipper), so I think I only need to acknowledge our shared agreement. Giving more information in support would currently be preaching to the choir. (But let me know if I incorrectly characterised your agreement, or what I claimed that you agreed to)

if you don't have a way to insert values, then you'll have to provide a way to create a node (a single node zipper) with a given value

Yes, that is true.

I feel like the insert method is more natural as you would just have to write something like insert_value(x) instead of insert(Node(x))

It seems like I can help by providing some possible sets of operations that we might want a zipper to support. This may help us decide what operations to test for. It seems like we'll always want to have left, right, up, and probably root; these are things that move the focus without changing the tree. I want to consider some possible operations we might want to support to change the focused tree.

• What does 1.1.0 currently support?
  • set_value(a, Zipper a) -> Zipper a
  • set_left(Tree a|null, Zipper a) -> Zipper a
  • set_right(Tree a|null, Zipper a) -> Zipper a
  • Athough this appears to be able to alter the three basic components of any binary tree, 1.1.0 doesn't have a clear answer for moving into an empty space and doing a set_left there (we don't have a value!), so that is a clear disadvantage of 1.1.0
• Current proposal of 2.0.0
  • insert(a, Zipper a) -> Zipper a
  • insert_zipper(Zipper a, Zipper a) -> Zipper a
  • set_value(a, Zipper a) -> Zipper a
  • Note that removing a subtree might imply that insert_zipper should work when the focus already has a node present!
• Proposal of http://learnyouahaskell.com/zippers
  • modify(a -> a, Zipper a) -> Zipper a
  • attach(Tree a, Zipper a) -> Zipper a
  • Note that attach works regardless of whether there is a node already present at the focus
  • One fewer function in the interface does mean that there's one fewer function to test, and hopefully less for the student to implement as well.
  • However, it's a completely valid point that this will require the ability to conveniently create a leaf node, for the case when we just want to insert a value. Therefore,student implementation work might be about equal
  • I honestly wonder whether modify is even necessary and whether it can just be written in terms of attach.

I'd like to see whether we can continue down this path of comparing some sets of operations the zipper would support, seeing what auxiliary operations we'd also need in order to be able to conveniently use those operations in reasonable ways, and see if anything stands out.

If there is no difference, that means the focus of the zipper does not affect the result, in which case providing the focus to be unnecessary information. The API would be clearer (takes in only what it needs) if it were only to take a tree.

Yes the focus doesn't make any difference, but I think it's clearer to insert zippers instead of trees. I think that's not unusual to have operations on an object that only consider a subset of the input characteristics. For example, when you write 12 mod 2 the mod function only look at the last bit of 12. In this case we would either have to write insert(z) or insert(z.tree), the first being shorter I tend to prefer it (note that the insert method will have to retrieve the tree anyway, the question is, who should retrieve it).

I request consideration of: In what situations are the various choices useful?
Consider a situation where trees are more plentiful than zippers.
If I have a tree and want to insert it into a zipper, it is more convenient to be able to provide it directly, rather than having to make a zipper out of the tree for the sole purpose of inserting that zipper into another zipper.
In a situation where zippers are more plentiful than trees, the opposite is true: It is more convenient to be able to provide the zipper directly, rather than having to extract the tree out of the zipper for the sole purpose of inserting it.

I'm making a wild speculation, without any supporting basis at all: I speculate that in typical uses of zippers, trees are more common than zippers.
If that is the case, we should favour having insert take in a tree, not a zipper.

And we have a high chance of running into this issue because many names are re-used in this JSON file

Shouldn't each test be encapsulated?

Yes they should, but many names are reused in the same case, so this problem happens even if each test case is self-contained.
However, it seems that we agree on the next point, so this point is moot:

This translates to something like (in python):
actual = zipper.left.up.value

Okay, it actualy looks like we agree on what the final representation of the test in a target language should look like.
In that case, please take my proposal as a way to reduce the temporary names (like tmp), while still preserving all the intent of your tests.
I think it will also have the side effect of making the tests easier to generate for generators of many languages as well.

is it really important that this JSON file is readable by a human?

Well, you and I are but human, and here we are discussing this file. I think it is inevitable.
We may also need to consider anyone who chooses not to make a generator but instead chooses to write the tests by hand after reading ths JSON file.

The idea that we should be able to move the zipper into an empty space is pretty crucial. It seems we've missed this throughout the entire history of having this exercise on Exercism ever since exercism/exercism#964 . As just one example, see from http://learnyouahaskell.com/zippers that this should obviously be a thing we should be able to do.

I'll have a look at this, for now I don't understand what you mean by "moving the zipper into an empty space".

Do not worry, I have a feeling that you already understand it. It is one of the major features you are proposing in this PR.
The idea that we should be able to move the focus to a position where there does not currently exist a node, but where we can insert one.

[petertseng]

So to recap:

I am 100% on board with the idea that we should be able to move the focus to a position that doesn't currently have a node and be able to insert at that focus.
In fact, if it's possible to have a 1.4.0 with just that idea, I want that merged ASAP.
(However, it may be tough to do that standalone because it raises questions about what to do with set_left/set_right at a focus that doesn't have a node)

I want us to think and discuss about the set of operations that zippers support to modify their focus. I've come to agree that set_left and set_right are not necessary; to replace them we want to figure out a good story around what type insert (or attach or whatever we want to call it) will take.

Even though I proposed changes to remove temporary names, it will save potential back-and-forth if we figure out what operations to support before figuring out how to encode all that in JSON.

[cglacet]

Even though I proposed changes to remove temporary names, it will save potential back-and-forth if we figure out what operations to support before figuring out how to encode all that in JSON.

Sure.

I request consideration of: In what situations are the various choices useful?
Consider a situation where trees are more plentiful than zippers. If I have a tree and want to insert it into a zipper, it is more convenient to be able to provide it directly, rather than having to make a zipper out of the tree for the sole purpose of inserting that zipper into another zipper. In a situation where zippers are more plentiful than trees, the opposite is true: It is more convenient to be able to provide the zipper directly, rather than having to extract the tree out of the zipper for the sole purpose of inserting it.

Yes I agree. I'm not sure if it translates well in every language, but maybe the best solution is to have a function that can take either a value, a tree or a zipper. In all these cases we can make insert to remove the existing tree/value if there is one. What I implemented in python is:

def insert(self, obj):
   focus = None
   if isinstance(obj, Zipper):
       focus = obj.tree
   elif isinstance(obj, Tree):
       focus = obj
   else:
       focus = Tree(obj, None, None) # A tree with both left and right children empty
   return Zipper(focus, self.context)

This way everyone is happy 😄 . But maybe it's a bit harder to understand for the students. That's mainly why I just kept things simple with having only the ability to insert node values.

[petertseng]

In all these cases we can make insert to remove the existing tree/value if there is one.

Yeah I've come to agree that this is a good thing.

maybe the best solution is to have a function that can take either a value, a tree or a zipper
This way everyone is happy 😄. But maybe it's a bit harder to understand for the students. That's mainly why I just kept things simple with having only the ability to insert node values.

This simplicity helps in more than one way. It also will help decrease the number the tests that we are obligated to write and decrease the amount of work required of a student who wishes to complete this exercise.

I think to be able to have the functionality of replacing entire subtrees, we need either the ability to insert a tree or to insert a zipper. We only need one of these two, and I have been thinking that inserting a tree makes more sense.

Now, I know that another desire stated in this PR was that there need not be any intermediate tree structures, which would argue for inserting a zipper. Personally I am thinking it is okay to have trees. Zippers exist as a way to manipulate a given data structure; it seems natural that when working with them, we will come into contact with the data structure they provide a view into (in this case, the tree).

Being able to conveniently insert a leaf just by specifying its value comes in handy to make the tests look simpler. I might propose that this should be a convenience that the tests should provide. By that, I mean: Students are only asked to implement inserting a tree. The tests have a convenience function that inserts a value into a zipper, by making a leaf node out of the given value. That way, students don't have to implement that.

[ErikSchierboom]

Seeing that this PR has been open for a long time, I was wondering what its status is. @petertseng you've added some comments in the above post. If I read your comment correctly, there is some work still left to do on this PR, right? If correct, are you still interested in doing said work @cglacet?

[cglacet]

I would like to, but currently I don't have any time for this.

[ErikSchierboom]

Okay, thanks for answering. Would you mind if I marked this PR as WIP then? That way, everyone can see that it is being worked on, but not ready to be merged.

[cglacet]

Yes sure no problem, I don't remember if I've linked it already, but I already have a working example of tests and a code example for the python implementation of this. Not everything in these two are up to date concerning the discussion we had here.

[ErikSchierboom]

I've marked this as WIP. Feel free to return to this when you have time again.

[ErikSchierboom]

@cglacet Are you still interested in working on this issue?

[cglacet]

Hey @ErikSchierboom, I think it's an interesting exercise and would love to improve it but I don't have time for this at the moment. I hope what I did in the past could be helpful here.

[ErikSchierboom]

Okay. @exercism/reviewers does anybody want to continue this work?