circular-buffer: Add canonical data

[petertseng]

The circular-buffer exercise was first added in 2014 May at #9, with
Ruby and Javascript as the initial implementing tracks:

• add circular buffer exercism DEPRECATED.javascript#17
• Circular buffer ruby#17

Implementing tracks:

• https://github.com/exercism/xcsharp/blob/master/exercises/circular-buffer/CircularBufferTest.cs
• https://github.com/exercism/xdlang/blob/master/exercises/circular-buffer/circular_buffer.d
• https://github.com/exercism/xecmascript/blob/master/exercises/circular-buffer/circular-buffer.spec.js
• https://github.com/exercism/xerlang/blob/master/exercises/circular-buffer/circular_buffer_tests.erl
• https://github.com/exercism/xfsharp/blob/master/exercises/circular-buffer/CircularBufferTest.fs
• https://github.com/exercism/xgo/blob/master/exercises/circular-buffer/circular_buffer_test.go
• https://github.com/exercism/xjavascript/blob/master/exercises/circular-buffer/circular-buffer.spec.js
• https://github.com/exercism/xlfe/blob/master/exercises/circular-buffer/test/circular-buffer-tests.lfe
• https://github.com/exercism/xlua/blob/master/exercises/circular-buffer/circular-buffer_spec.lua
• https://github.com/exercism/xpascal/blob/master/exercises/circular-buffer/uCircularBufferTests.pas
• https://github.com/exercism/xpython/blob/master/exercises/circular-buffer/circular_buffer_test.py
• https://github.com/exercism/xruby/blob/master/exercises/circular-buffer/circular_buffer_test.rb
• https://github.com/exercism/xrust/blob/master/exercises/circular-buffer/tests/circular-buffer.rs

All tracks pretty much implement the same tests, except:

• Tracks typically have the "read item just written" and "each item can
  only be read once" tests combined. These tests split the two.
• In dynamically-typed languages, buffers may be able to store
  heterogeneous types. This concept is not expressed in these tests.
• In some statically-typed languages, buffers use generics, such that
  buffers may store any one type of the client's choosing. This concept
  is also not expressed in these tests.
• The final test (ensuring that overwrite drops the right items) was more
  complex: capacity 5, 3 writes, 2 reads, write, read, 4 writes, 2
  overwrites, 5 reads. It's been simplified to capacity 3, 3 writes, 1
  read, write, overwrite, 3 reads.

Closes exercism/todo#79

[Insti]

s/Rail/Fail/

[petertseng]

Excuse me! I obviously meant "complain or protest strongly and persistently about", meaning the code should rail at its user on an attempt to perform a read.

(Fixed)

[petertseng]

not related to reading an item just written. A separate test, "Each item can only be read once"?

[petertseng]

(our comments remain, but this comment has been addressed, as this line now goes under "each item may only be read once")

[petertseng]

Not related to reading items in the order they are written.

[Insti]

This is part of a different test: "cannot read past the end of buffer" or something

[Insti]

redundant operation.

[Insti]

I think what you're doing here is good. 👍

[petertseng]

s/fills/frees/

[petertseng]

All these commits are going to be squashed and I'll amend the commit message and PR description to be reasonable.

[petertseng]

New tests added:

• "items cleared out of buffer can't be read"
• "overwrite doesn't remove an already-read item" - As first commit explains, a simplified version of a test mixing reads, writes, overwrites, but the main point as I see it was to make sure that overwrite deletes the right item if a read has previosly been performend

[petertseng]

Note the addition of this, which should look suspiciously similar to https://github.com/exercism/x-common/blob/master/exercises/react/canonical-data.json#L38-L40

Is this valuable to point out? Well, some tracks already have it, and I didn't want this JSON file to imply that they should remove those tests or make their test suites discourage generics. In the latter case, tracks may even get questions.

[petertseng]

https://github.com/petertseng/x-common/commit/f882899ef955323c4e38f977df697b8b1f9b098d

Updated: now https://github.com/petertseng/x-common/commit/fbe9566ebedec7ea064bdb655a02120b4d7164d7 after should_succeed change

[Insti]

I wonder if this should be:

"operation": "read"
"expected": false

Ensuring that there is an expected result for each of the 4 methods.

Another example for writeShouldFail

"operation": "write"
"item": 1
"expected": false

Or is this being too proscriptive of implementation details?

[petertseng]

Okay, I think I understand.

A few things give me pause about the "expected": false to signal failure on read:

• Not a concern: now we can't have a buffer of booleans and write false into it and expect to read a false out! (not relevant for this JSON file since it only deals with integers)
• The explanation of read gets a little gnarly. It becomes "read: Reading from the buffer should produce the item located at expected, unless expected is false, in which case the read should fail."
• Statically-typed languages reading this JSON file may have more trouble dealing with the fact that the value at expected might be either a boolean or an integer.

Okay, what if it's something like this: for a successful read of 1:

{
  "operation": "read",
  "item": 1,
  "expected": true,
}

and for a failing read:

{
  "operation": "read",
  "expected": false,
}

this way, expected is a boolean for all cases.

I'm not convinced I'm happy with the name expected in this usage, since now it doesn't make clear that the main thing we want to check is the value read, rather than whether it was successful. What if expected got renamed to success or shouldSucceed?

I'm not sure if this is also part of the suggestion, but I would feel odd about adding an expected: key to overwrite and clear, since having it there implies that it can be false, and I don't think we'll ever have a test that expects either of these two to fail (well, maybe overwrite on a size-0 buffer, but I think I'd rather argue that size-0 buffers should just not get created)

I am having a hard time with the "expected": false for reads, and I'm not sure that moving the item to a separate key makes things better. Any way to make this work?

---

This has a little to do with #401, what's the best way to represent an error here. I am having a sufficiently hard time with fitting "expected": true/false into all operations that I think I should keep them separate. This is the suggestion of #401 (comment) - errors are simply encoded differently. I know I didn't use that in any other JSON file I've written, but I managed to make null work in all of those, whereas I'm unsure whether integer/null for reads versus true/false for writes gets too confusing. Even integer/false for reads seems a bit odd.

That, and I may be a bit biased by how the go tests were written - they pretty much use those exact six operations already.

[Insti]

Command Query Responsibility Segregation.

write is a command.
overwrite is a command.
clear is a command.
read is a query.

detecting failure is a query.

It seems we are missing the failure_detected* query that indicates this.

  "cases": [
    {
      "description": "reading empty buffer should fail",
      "capacity": 1,
      "operations": [
        {
          "operation": "read",
          "expected": null
        },
        {
          "operation": "failure_detected",
          "expected": true
        }
      ]
    },

Individual tracks are still free to implement the failure_detected check however they like, either with a method or a try/catch block or whichever other system their language encourages.

* There's probably a better name for this.

[petertseng]

Even though tracks are free to implement failure_detected how they like, I am guessing that failure_detected as a separate query will tend to suggest that it be a separate function. After all, someone porting this exercise to their track might simply think "one function for each operation, right?!", because what reason do they have to think that this failure_detected operation is any different than the others (read, write, overwrite, clear)*?

This hypothetical API with a separate failure_detected operation is one I do not wish to encourage porters to implement, especially since students will subsequently see it, and be encouraged to replicate it. I tend to think it's better to make failures clear right when they happen. After all, both queries and commands have the potential of failing, so it seems cumbersome to have to check failure in a separate query. A separate failure_detected query also seems difficult to make work in potentially concurrent contexts, but luckily that is not a concern for this particular exercise!

I have a difficult time fitting in a circular buffer with command-query separation, even without failure_detected. I see from https://en.wikipedia.org/wiki/Command%E2%80%93query_separation that command-query separation has the formal definition of "methods should return a value only if they are referentially transparent and hence possess no side effects."

The read operation has a side-effect, since it removes the just-read item from the buffer!
If we wish to follow command-query separation, read is not allowed to both return the oldest value and also remove it.
Instead, we would be forced to separate the read operation into peek and dropOldestItem.
Well, hey, I might not be opposed to that API change, but I unfortunately don't think I will try to convince others that it is the right thing to do for their tracks.

*: Hoewver, I have just realized after writing this argument against failure_detected that the current JSON schema also has two "special' operations: readShouldFail and writeShouldFail. What's to tell a porter that those two operations are in any way special? ARGH! So is my proposed JSON schema flawed as well?!

[Insti]

The read operation has a side-effect, since it removes the just-read item from the buffer!

This is a good point.

[petertseng]

(I have changes planned to remove readShouldFail and writeShouldFail since in arguing against failure_detected I managed to argue against the forme rtwo as well)

[Insti]

Do we need to say anything like 'failure may be indicated by raising an exception or similar language dependent runtime error'

[petertseng]

I added it just to be sure. I have a nagging feeling that if similar sentences appear in too many places...

• https://github.com/exercism/x-common/blob/master/exercises/largest-series-product/canonical-data.json#L3
• https://github.com/exercism/x-common/blob/master/exercises/hamming/canonical-data.json#L8
• https://github.com/exercism/x-common/blob/master/exercises/rna-transcription/canonical-data.json#L6
• https://github.com/exercism/x-common/blob/master/exercises/triangle/canonical-data.json#L15

then we might extract them all out to the contributing guide's section on porting an exercise. But I don't know what the right answer to that is yet, so for now I'll add the sentence.

(or maybe we are worried that a porter will not read that particular section of the contributing guide? I don't know)

[Insti]

anecdata: When I'm porting, I tend to just read the canonical-data and the readme.
I don't re-read the contributing guide often.
(I'm probably non-typical enough for this to be irrelevant information.)