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

list-ops: append descriptions do not match inputs #1609

Closed
wolf99 opened this issue Oct 15, 2019 · 4 comments
Closed

list-ops: append descriptions do not match inputs #1609

wolf99 opened this issue Oct 15, 2019 · 4 comments

Comments

@wolf99
Copy link
Contributor

wolf99 commented Oct 15, 2019

In the canonical-data.json for the list-ops exercise test case 2 and 3 have descriptions that are conflicting, given the associated inputs.

In the append entries to a list and return the new list cases:

  • Test case 2: "empty list to list" expects list1 input to be appended to list2 input
  • Test case 3: "non-empty lists" expects list2 input to be appended to list1 input

(This is based on the description and the ordering of the values in the expected output..)

Any given implementation can only either append or prepend, but not arbitrarily chose one of append or prepend. Given the name of the cases relates to appending, I understand that append is what was intended, but this is not what these test cases imply.

Additional notes on reading the spec by someone who doesn't know functional operations:

  • In the foldl and foldr test cases it seems ambiguous as to which argument in the function is to be used for the initial value, i.e. is the value from the list x or y? This makes understanding the division operations harder than it should be.
  • when concatenating a list of nested lists is it intended that the empty list is not removed completely as it was in the list of lists? I guess the implication is that it is a "shallow" operation?
@wolf99
Copy link
Contributor Author

wolf99 commented Oct 15, 2019

Problem: (these orderings are at odds with each other)

        {
          "description": "empty list to list",  //<-- the ordering described
          "property": "append",
          "input": {
            "list1": [], // <-- the ordering of the inputs
            "list2": [1, 2, 3, 4]
          },
          "expected": [1, 2, 3, 4]
        },
        {
          "description": "non-empty lists",
          "property": "append",
          "input": {
            "list1": [1, 2], // <-- the ordering of the inputs
            "list2": [2, 3, 4, 5]
          },
          "expected": [1, 2, 2, 3, 4, 5] // <-- the ordering of the expected result
        }

Solution proposal 1:

        {
          "description": "list to empty list",  //<-- Change description
          "property": "append",
          "input": {
            "list1": [],
            "list2": [1, 2, 3, 4]
          },
          "expected": [1, 2, 3, 4]
        },
        {
          "description": "non-empty lists",
          "property": "append",
          "input": {
            "list1": [1, 2],
            "list2": [2, 3, 4, 5]
          },
          "expected": [1, 2, 2, 3, 4, 5]
        }

Solution proposal 2:

        {
          "description": "empty list to list",
          "property": "append",
          "input": {
            "list1": [],
            "list2": [1, 2, 3, 4]
          },
          "expected": [1, 2, 3, 4]
        },
        {
          "description": "non-empty lists",
          "property": "append",
          "input": {
            "list1": [1, 2],
            "list2": [2, 3, 4, 5]
          },
          "expected": [2, 3, 4, 5, 1, 2]  //<-- Change expected result ordering
        }

Solution proposal 3:

        {
          "description": "empty list to list",
          "property": "append",
          "input": {
            "list1": [1, 2, 3, 4]  //<-- change order of inputs
            "list2": [],
          },
          "expected": [1, 2, 3, 4]
        },
        {
          "description": "non-empty lists",
          "property": "append",
          "input": {
            "list1": [1, 2],
            "list2": [2, 3, 4, 5]
          },
          "expected": [1, 2, 2, 3, 4, 5]
        }

Solution 3 seems like the best to me.

@petertseng
Copy link
Member

petertseng commented Oct 15, 2019

I see. Changing the description to "list to empty list" (solution 1) seems to be the least work, since it changes no inputs and makes everything consistent.

I think solution 2 looks a little unintuitive given the name of append.

It does not appear that I need to specify my preference between solution 1 or 3 at this moment.


A potential future direction: One could argue that both cases are worth testing.

          "input": {
            "list1": [],
            "list2": [1, 2, 3, 4]
          },
          "input": {
            "list1": [1, 2, 3, 4],
            "list2": []
          },

I would surmise that fixing the current test (whatever that fix may be) is not embargoed, but adding a new test would be. Therefore, my suggestion would be to submit those as separate PRs, so that the fix can be merged when it gets the approvals, and the additional test can be merged after the embargo is lifted.

@yawpitch
Copy link
Contributor

* Test case 2: "empty list _to_ list" expects `list1` input to be appended to `list2` input

* Test case 3: "non-empty lists" expects `list2` input to be appended to `list1` input

To my reading both expect the contents of list2 to be appended to the end of list1, as described in the copy in the description.md:

append (given two lists, add all items in the second list to the end of the first list)

As described [] ++ [1] is an append and [1] ++ [] is a prepend, even if they both give the same result. In a language with mutation, though, there's a significant difference between the two.

Solution 2 would, therefore, represent a prepend, since the items in the second list end up before the items in the first. And, in a language with mutation, the identity of the object returned would be incorrect. As stated there's no real ambiguity then with the non-empty lists test.

The proposed Solution 1 does seem to improve the meaning since the description becomes "[append a non-empty] list to [an] empty list", which is easier to understand than the existing description (which is indeed ambiguous) so I'd tend to vote for that one.

Solution 3 is somewhat moot because if the existing case isn't also included, as per @petertseng ... however both the proposed Solution 3 and adding the mirror case would currently be blocked by #1560.

@yawpitch
Copy link
Contributor

Given that this issue is about the order in the description it’s closed by the merge of #1611. The additional case PR in #1612 will remain open but on hold.

sshine pushed a commit to exercism/tcl that referenced this issue Mar 10, 2020
Per exercism/problem-specifications#1609, this change makes the ordering of 
inputs implied by the description match the inputs as they are actually supplied.
This bumps the version from 2.4.0 to 2.4.1.

This change reflects exercism/problem-specifications#1611
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

No branches or pull requests

3 participants