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:
list1 input to be appended to list2 inputlist2 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:
x or y? This makes understanding the division operations harder than it should be.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.
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.
* 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.
Given that this issue is about the order in the description it鈥檚 closed by the merge of #1611. The additional case PR in #1612 will remain open but on hold.
Most helpful comment
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.
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.