Implemented circular buffer #31

Closed
wants to merge 16 commits into
from

Projects

None yet

2 participants

Contributor
dglol commented Jan 22, 2012

Some particulars I need feedback for:

  • Whether the size checking is necessary. (Line 28 of circular-buffer.js)
  • Whether or not I am using events properly or not (Lines 74, 91 of circular-buffer.js)
  • The usefulness of some auxiliary methods (.back(), .clear(), .dequeue())
Contributor
dglol commented Jan 22, 2012

Test results:

Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/xz/yq9z4c4x76d1f3d1l9w4z4280000gn/T/tmpvJkYhZ.mozrunner'.
Running tests on Firefox 10.0/Gecko 10.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under darwin/x86.
info: executing 'test-buffer.test_clear'
info: pass: Buffer should be empty after a clear
info: executing 'test-buffer.test_readwrite'
info: pass: An empty read should be undefined
info: pass: Buffer should be empty after a read without content
info: pass: GC field in retrieved data should be 2
info: pass: Buffer should technically be empty after a reading all data
info: pass: GC field in last written data should be 8
info: pass: GC field in retrieved data should be 4
info: pass: GC field in retrieved data should be 5
info: pass: GC field in retrieved data should be 6
info: pass: GC field in retrieved data should be 7
info: pass: GC field in retrieved data should be 8
info: pass: Buffer should technically be empty after a reading all data
info: executing 'test-buffer.test_state'
info: pass: Size of buffer should be 10
info: pass: Buffer should be empty
info: pass: Buffer should not be full
info: pass: Buffer should not be empty
info: pass: Buffer should not be full
info: pass: Buffer should be full

18 of 18 tests passed.
Total time: 1.442964 seconds
Program terminated successfully.
@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+ constructor: function CircularBuffer(length) {
+ // Report unhandled errors from listeners
+ this.on("error", console.exception.bind(console));
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+
+ this._size = length;
+ this._head = 0;
+ this._tail = 0;
+ this._buffer = [];
+ this._isLastOpAWrite = false;
+
+ // Ensure a minimum length of 1
+ if (typeof(length) == 'undefined' || length < 1)
+ this._size = 1;
whimboo
whimboo Jan 23, 2012 Contributor

Move the check for arguments up to the top of the function.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+
+ this._size = length;
+ this._head = 0;
+ this._tail = 0;
+ this._buffer = [];
+ this._isLastOpAWrite = false;
+
+ // Ensure a minimum length of 1
+ if (typeof(length) == 'undefined' || length < 1)
+ this._size = 1;
+ },
+
+ size: function CircularBuffer_size() {
whimboo
whimboo Jan 23, 2012 Contributor

This and other instances should become a property.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
@@ -0,0 +1,114 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { EventEmitter } = require("api-utils/events");
+const unload = require("api-utils/unload");
+
+const ON_READ = "bufferRead";
whimboo
whimboo Jan 23, 2012 Contributor

Not sure yet if we need this. See further comments below.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+ this._size = 1;
+ },
+
+ size: function CircularBuffer_size() {
+ return this._size;
+ },
+
+ unload: function CircularBuffer_unload() {
+ this._removeAllListeners(ON_WRITE);
+ },
+
+ // Returns the next
+ nextIndex: function CircularBuffer_nextIndex(index) {
+ if (index >= (this._size - 1))
+ return 0;
+ return (index + 1);
whimboo
whimboo Jan 23, 2012 Contributor

Use the module operator here which will make it easier with overflows.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+
+const ON_READ = "bufferRead";
+const ON_WRITE = "bufferWrite";
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(length) {
+ // Report unhandled errors from listeners
+ this.on("error", console.exception.bind(console));
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+
+ this._size = length;
+ this._head = 0;
+ this._tail = 0;
+ this._buffer = [];
whimboo
whimboo Jan 23, 2012 Contributor

The buffer should already be created with the number of elements specified by length.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+
+ prevIndex: function CircularBuffer_prevIndex(index) {
+ if (index <= 0)
+ return (this._size - 1);
+ return (index - 1);
+ },
+
+ // Reads earliest inserted data and returns it
+ read: function CircularBuffer_read() {
+ const DATA = this._buffer[this._head];
+
+ if (this.isEmpty())
+ return 'undefined';
+ this._head = this.nextIndex(this._head);
+ this._isLastOpAWrite = false;
+ this._emit(ON_READ);
whimboo
whimboo Jan 23, 2012 Contributor

I don't think that we need to emit on read. A write should be the only important piece.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+
+ // Returns the next
+ nextIndex: function CircularBuffer_nextIndex(index) {
+ if (index >= (this._size - 1))
+ return 0;
+ return (index + 1);
+ },
+
+ prevIndex: function CircularBuffer_prevIndex(index) {
+ if (index <= 0)
+ return (this._size - 1);
+ return (index - 1);
+ },
+
+ // Reads earliest inserted data and returns it
+ read: function CircularBuffer_read() {
whimboo
whimboo Jan 23, 2012 Contributor

I miss an index to read from. Looks like that we should have worked out the specs before an implementation.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+ this._head = this.nextIndex(this._head);
+ this._isLastOpAWrite = false;
+ this._emit(ON_READ);
+ return DATA;
+ },
+
+ // Reads earliest inserted data, removes it, and returns it
+ dequeue: function CircularBuffer_dequeue() {
+ const DATA = this._buffer[this._head];
+
+ if (this.isEmpty())
+ return 'undefined';
+ this._buffer[this._head] = 'undefined';
+ this._head = this.nextIndex(this._head);
+ this._isLastOpAWrite = false;
+ this._emit(ON_READ);
whimboo
whimboo Jan 23, 2012 Contributor

Mostly the same code as in read(). It could be called internally.

I like it that we have two separate methods because not all read requests should remove entries from the buffer.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/lib/circular-buffer.js
+ },
+
+ // Reads last inserted data and returns it
+ back: function CircularBuffer_back() {
+ if (this.isEmpty())
+ return 'undefined';
+ return this._buffer[this.prevIndex(this._tail)];
+ },
+
+ write: function CircularBuffer_write(data) {
+ if (this.isFull())
+ this._head = this.nextIndex(this._head);
+ this._buffer[this._tail] = data;
+ this._tail = this.nextIndex(this._tail);
+ this._isLastOpAWrite = true;
+ this._emit(ON_WRITE);
whimboo
whimboo Jan 23, 2012 Contributor

When you send this event, you also want to send the value which has been added.

@whimboo whimboo commented on an outdated diff Jan 23, 2012
extension/tests/test-buffer.js
@@ -0,0 +1,63 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const circular_buffer = require("circular-buffer");
whimboo
whimboo Jan 23, 2012 Contributor

wohoo tests... that's nice!

Contributor
whimboo commented Jan 23, 2012

As mentioned earlier please lets come together on a table and define the specs for this module first before we continue to work on it. May I asked where you have taken this code from?

Contributor
dglol commented Jan 23, 2012

The code is all written from scratch with concepts taken from wikipedia (http://en.wikipedia.org/wiki/Circular_buffer). Thanks for the feedback; we'll talk soon.

Forgot to reset length back to the original size. This will be fixed in the next commit.

Contributor
dglol commented Jan 31, 2012

Test Results:

Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/xz/yq9z4c4x76d1f3d1l9w4z4280000gn/T/tmpUcEUQK.mozrunner'.
Running tests on Firefox 10.0/Gecko 10.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under darwin/x86.
info: executing 'test-buffer.test_back_ops'
info: pass: GC field in popped data should be 4
info: pass: GC field in popped data should be 5
info: pass: GC field in popped data should be 6
info: pass: GC field in popped data should be 7
info: pass: GC field in popped data should be 8
info: pass: GC field in shifted data should be undefined
info: pass: Buffer should be empty
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 16 column: 50 source: "      this._dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 16 column: 50 source: "      this._dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 16 column: 50 source: "      this._dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0777);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 25 column: 44 source: "    foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 25 column: 44 source: "    foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 25 column: 44 source: "    foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 49 column: 51 source: "      foStream.init(this.file, 0x02 | 0x08 | 0x10, 0666, 0);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 49 column: 51 source: "      foStream.init(this.file, 0x02 | 0x08 | 0x10, 0666, 0);
"}]
console: [JavaScript Warning: "octal literals and octal escape sequences are deprecated" {file: "resource://memchaser-at-quality-dot-mozilla-dot-org/api-utils/lib/cuddlefish.js -> resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/lib/logger.js" line: 49 column: 51 source: "      foStream.init(this.file, 0x02 | 0x08 | 0x10, 0666, 0);
"}]
info: executing 'test-buffer.test_clear'
info: pass: An empty read should be undefined
info: pass: Buffer's size should be restored
info: pass: Buffer should be empty
info: executing 'test-buffer.test_front_ops'
info: pass: GC field in shifted data should be 8
info: pass: GC field in shifted data should be 7
info: pass: GC field in shifted data should be 6
info: pass: GC field in shifted data should be 5
info: pass: GC field in shifted data should be 4
info: pass: GC field in shifted data should be undefined
info: pass: Buffer should be empty
info: executing 'test-buffer.test_readwrite'
info: pass: An empty read should be undefined
info: pass: GC field in retrieved data should be 2
info: pass: GC field in last written data should be 8
info: pass: GC field in retrieved data should be 7
info: pass: GC field in retrieved data should be 6
info: pass: GC field in retrieved data should be 5
info: pass: GC field in retrieved data should be 4
info: pass: GC field in retrieved data should be 5
info: pass: GC field in retrieved data should be 6
info: pass: GC field in retrieved data should be 7
info: pass: GC field in retrieved data should be 8
info: executing 'test-buffer.test_state'
info: pass: Size of buffer should be 10
info: pass: Buffer should not be full
info: pass: Buffer should not be full
info: pass: Buffer should be full
info: executing 'test-logger.test_default_to_not_logging'
info: pass: assertion successful
info: executing 'test-logger.test_start_stop_logging'
debug: Logging to '/Users/dguo/Library/Caches/TemporaryItems/1327997911000.log' started.
info: pass: assertion successful
debug: Logging to '/Users/dguo/Library/Caches/TemporaryItems/1327997911000.log' stopped.
info: pass: assertion successful
warning: 9 warnings or errors were logged to the platform's nsIConsoleService, which could be of no consequence; however, they could also be indicative of aberrant behavior.

35 of 35 tests passed.
Total time: 1.512570 seconds
Program terminated successfully.
@whimboo whimboo commented on an outdated diff Feb 5, 2012
extension/lib/circular-buffer.js
@@ -0,0 +1,165 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+const ON_WRITE = 'bufferWrite';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(length) {
whimboo
whimboo Feb 5, 2012 Contributor

To be conform with the SDK specifications the parameters of the constructor have to be an object and not a list of single parameters.

@whimboo whimboo and 1 other commented on an outdated diff Feb 5, 2012
extension/lib/circular-buffer.js
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+const ON_WRITE = 'bufferWrite';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(length) {
+ if (typeof(length) == 'undefined' || length < 1) {
+ throw {
+ name: 'Input Error',
+ message: 'Input not declared or out of bounds.'
+ };
whimboo
whimboo Feb 5, 2012 Contributor

Instead of throwing an exception we should set a default length for the buffer. What would you say about 100 elements?

dglol
dglol Feb 8, 2012 Contributor

For my next commit I am going to default to 60 elements (the interval of an entire minute split into seconds), and throw an error if it is an invalid input (i.e. a negative integer).

@whimboo whimboo and 1 other commented on an outdated diff Feb 5, 2012
extension/lib/circular-buffer.js
+
+ this._front = 0;
+ this._back = 0;
+ this._buffer = [];
+ this._buffer.length = this._size;
+ this._dataWritten = false;
+
+ // Report unhandled errors from listeners
+ this.on('error', console.exception.bind(console));
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+ },
+
+ // Returns the next index, adjusted for cycles
+ _nextIndex: function CircularBuffer_nextIndex(index) {
whimboo
whimboo Feb 5, 2012 Contributor

Private methods would need a double underscore for the internal name.

dglol
dglol Feb 8, 2012 Contributor

I'm rejecting this suggestion for now. From what I understand, traits will hide the method name with a single underscore. It might be a python/PHP idiom but I feel that it is tacky for this.

@whimboo whimboo commented on an outdated diff Feb 5, 2012
extension/lib/circular-buffer.js
+
+ // Returns the next index, adjusted for cycles
+ _nextIndex: function CircularBuffer_nextIndex(index) {
+ return (index + 1) % this._size;
+ },
+
+ // Returns the previous index, adjusted for cycles
+ _prevIndex: function CircularBuffer_prevIndex(index) {
+ index = (index - 1) %this._size;
+ if (index < 0) {
+ index += this._size;
+ }
+ return index;
+ },
+
+ size: function CircularBuffer_size() {
whimboo
whimboo Feb 5, 2012 Contributor

Please make this a getter so it can be used as buffer.size without brackets.

@whimboo whimboo commented on an outdated diff Feb 5, 2012
extension/lib/circular-buffer.js
+ this._dataWritten = true;
+ this._emit(ON_WRITE, data);
+ },
+
+ // Reads data from the front, removes it, and returns it
+ shift: function CircularBuffer_shift() {
+ const DATA = this.read(0);
+ if (typeof(DATA) === 'undefined') {
+ return undefined;
+ }
+
+ this._buffer[this._front] = undefined;
+ this._front = this._nextIndex(this._front);
+ this._dataWritten = false;
+
+ return DATA;
whimboo
whimboo Feb 5, 2012 Contributor

I know that I have mentioned we can remove READ, but I think it would be great to send a REMOVE event here to notify possible listeners. Could be helpful in the future.

Contributor
whimboo commented Feb 10, 2012

David, it would also be great to have a way to resize the max number of elements for the buffer.

Contributor
dglol commented Feb 11, 2012

In the case where you resize to a smaller
Would it:

  • Truncate the last (old_size - new_size) buffers
  • Shift the old buffer to the new buffer
  • Clear the entire buffer and resize it at it's new length
Contributor
whimboo commented Feb 15, 2012

We do not want to loose any recent data if the buffer gets smaller. So the oldest entries would have to be removed.

I also would like to see feedback from @geoelectric on this pull request.

Contributor
dglol commented Mar 14, 2012

Resizing is in the latest commit.

Contributor
whimboo commented Mar 23, 2012

Thanks for the latest updates on it @dglol. Would you mind to implement the code which exchanges the current gData object with the circular buffer? That would allow us to better test this new major feature/

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+const ON_WRITE = 'bufferWrite';
+const ON_REMOVE = 'bufferRemove';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(aOptions) {
+ this._length = aOptions.length || 60;
+ if (this._length < 1) {
+ throw {
+ name: 'RangeError',
+ message: 'Invalid length'
+ };
whimboo
whimboo Mar 23, 2012 Contributor

Can we define this type of Error as a new Error class? We could do it similar as what we wanna do for the mozmill-tests API refactor:
https://github.com/geoelectric/mozmill-api-refactor/blob/master/lib/api/errors.js

dglol
dglol Mar 23, 2012 Contributor

Can you clarify what you mean by this? I chose RangeError because it's commonly used for this type of error: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/RangeError .

whimboo
whimboo Mar 23, 2012 Contributor

You are not throwing a RangeError but a nameless object, which has a name property set to 'RangeError'. Please throw an instance of the RangeError class instead. Good to know that this already exists.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
@@ -0,0 +1,207 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+const ON_WRITE = 'bufferWrite';
+const ON_REMOVE = 'bufferRemove';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(aOptions) {
+ this._length = aOptions.length || 60;
whimboo
whimboo Mar 23, 2012 Contributor

aOptions could be undefined. So please ensure it's at least an empty hash.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ return this._count;
+ },
+
+ get length() {
+ return this._length;
+ },
+
+ set length(aNewLength) {
+ if (aNewLength < 0) {
+ throw {
+ name: 'RangeError',
+ message: 'Invalid length'
+ };
+ }
+
+ // L51-63: Normalize the circular buffer as a FIFO buffer
whimboo
whimboo Mar 23, 2012 Contributor

Don't reference lines because those will change over time and will make the comment invalid.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ throw {
+ name: 'RangeError',
+ message: 'Invalid length'
+ };
+ }
+
+ // L51-63: Normalize the circular buffer as a FIFO buffer
+ if (this._front > this._back) {
+ this._buffer.push.apply(this._buffer,
+ this._buffer.slice(this._front));
+ this._buffer.push.apply(this._buffer,
+ this._buffer.slice(this._back,this._front));
+ }
+ else {
+ this._buffer.push.apply(this._buffer,
+ this._buffer.slice(this._front,this._back));
whimboo
whimboo Mar 23, 2012 Contributor

nit: always use a space after a comma. It applies to a couple of instance in this file.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+
+ this._buffer = this._buffer.slice(0,this._length);
+
+ // Truncate the oldest elements if new length is less than original
+ if (aNewLength < this._length) {
+ this._buffer = this._buffer.slice(this._length - aNewLength);
+ }
+
+ this._buffer.length = aNewLength;
+ this._length = aNewLength;
+ this._front = 0;
+ this._count = Math.min(this._count, aNewLength);
+ this._back = this._nextIndex(this._count - 1);
+ },
+
+ // Returns the next index, adjusted for cycles
whimboo
whimboo Mar 23, 2012 Contributor

If you want to add a comment for a method please JSDoc complient style '/** ... */'

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ _prevIndex: function CircularBuffer_prevIndex(aIndex) {
+ aIndex = (aIndex - 1) % this._length;
+ if (aIndex < 0) {
+ aIndex += this._length;
+ }
+ return aIndex;
+ },
+
+ unload: function CircularBuffer_unload() {
+ this._removeAllListeners(ON_WRITE);
+ },
+
+ // Reads indexed data and returns it
+ // If no index is specified, reads data from the front and returns it
+ read: function CircularBuffer_read(aIndex) {
+ if (typeof(aIndex) === 'undefined') {
whimboo
whimboo Mar 23, 2012 Contributor

'aIndex === undefined' should be enough, or why not:

aIndex = aIndex || 0

Beside that I don't really like it to modify parameters. so better assign it to a new local variable?

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ }
+
+ // Adjust the index to imitate python-style indexing
+ if (aIndex >= 0) {
+ aIndex = (this._front + aIndex) % this._length;
+ }
+ else {
+ aIndex = (this._back + aIndex) % this._length;
+ if (aIndex < 0) {
+ aIndex += this._length;
+ }
+ }
+
+ const DATA = this._buffer[aIndex];
+
+ return DATA;
whimboo
whimboo Mar 23, 2012 Contributor

Why const? After we return from the function it will be a normal variable or?

dglol
dglol Mar 23, 2012 Contributor

It will be a normal variable, but you're right -- the const isnt as important as I thought it would be here. I think this was just habits from C++ but I'll change this :p.

whimboo
whimboo Mar 23, 2012 Contributor

Alright. Good to know. We are on the right track then...

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ unshift: function CircularBuffer_unshift(aData) {
+ if (this.isFull()) {
+ this._back = this._prevIndex(this._back);
+ }
+ else {
+ this._count += 1;
+ }
+
+ this._front = this._prevIndex(this._front);
+ this._buffer[this._front] = aData;
+ this._emit(ON_WRITE, aData);
+ },
+
+ // Reads data from the front, removes it, and returns it
+ shift: function CircularBuffer_shift() {
+ const DATA = this.read(0);
whimboo
whimboo Mar 23, 2012 Contributor

Same here. Why does it have to be a const?

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+
+ return DATA;
+ },
+
+ // Clears data from the buffer and resets
+ clear: function CircularBuffer_clear() {
+ this._buffer.length = 0;
+ this._buffer.length = this._length;
+ this._front = 0;
+ this._back = 0;
+ this._count = 0;
+ this._emit(ON_REMOVE);
+ },
+
+ isEmpty: function CircularBuffer_isEmpty() {
+ return this._count === 0;
whimboo
whimboo Mar 23, 2012 Contributor

nit:please use brackets around the comparison

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
@@ -0,0 +1,208 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const circular_buffer = require("circular-buffer");
whimboo
whimboo Mar 23, 2012 Contributor

Do a 'const {CircularBuffer} = ...' here

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
@@ -0,0 +1,208 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const circular_buffer = require("circular-buffer");
+
+exports.test_state = function(test) {
+ var buffer = circular_buffer.CircularBuffer({length: 10});
+
+ test.assertEqual(buffer.length, 10, "Size of buffer should be 10");
+ test.assert(!buffer.isFull(), "Buffer should not be full");
+
+ buffer.write({resident:1,GC:2,CC:3});
whimboo
whimboo Mar 23, 2012 Contributor

would you mind using default names for properties here? It would be better portable to other projects then.

dglol
dglol Mar 23, 2012 Contributor

Definitely, hadn't thought about that.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+
+exports.test_readwrite = function(test) {
+ var buffer = circular_buffer.CircularBuffer({length: 5});
+
+ test.assertEqual(buffer.read(), undefined, "An empty read should be undefined");
+
+ buffer.write({resident:1,GC:2,CC:3});
+ test.assertEqual(buffer.read().GC, 2, "GC field in retrieved data should be 2");
+
+ buffer.write({resident:1,GC:2,CC:3});
+ buffer.write({resident:1,GC:3,CC:3});
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ // Should overwrite the first write
whimboo
whimboo Mar 23, 2012 Contributor

I would say 'removes the entry from the first write'

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ test.assertEqual(buffer.read().GC, 2, "GC field in retrieved data should be 2");
+
+ buffer.write({resident:1,GC:2,CC:3});
+ buffer.write({resident:1,GC:3,CC:3});
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ // Should overwrite the first write
+ buffer.write({resident:1,GC:7,CC:3});
+
+ // Should overwrite the second write
+ buffer.write({resident:1,GC:8,CC:3});
+
+ test.assertEqual(buffer.read(-1).GC, 8, "GC field in last written data should be 8");
+ test.assertEqual(buffer.read(-2).GC, 7, "GC field in retrieved data should be 7");
whimboo
whimboo Mar 23, 2012 Contributor

sweet!

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ // Should overwrite the first write
+ buffer.write({resident:1,GC:7,CC:3});
+
+ // Should overwrite the second write
+ buffer.write({resident:1,GC:8,CC:3});
+
+ test.assertEqual(buffer.read(-1).GC, 8, "GC field in last written data should be 8");
+ test.assertEqual(buffer.read(-2).GC, 7, "GC field in retrieved data should be 7");
+ test.assertEqual(buffer.read(-3).GC, 6, "GC field in retrieved data should be 6");
+ test.assertEqual(buffer.read(-4).GC, 5, "GC field in retrieved data should be 5");
+
+ test.assertEqual(buffer.read().GC, 4, "GC field in retrieved data should be 4");
whimboo
whimboo Mar 23, 2012 Contributor

Hm, we removed two entries which had GC==1 and GC==2. So the oldest should be GC==3. Why is that 4 here?

dglol
dglol Mar 23, 2012 Contributor

The result is correct but the comments are not. Missed the first .write which was overwritten by GC: 6 line. I'll fix that.

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ test.assertEqual(buffer.pop(), undefined, "GC field in shifted data should be undefined");
+ test.assert(buffer.isEmpty(), "Buffer should be empty");
+};
+
+exports.test_clear = function(test) {
+ var buffer = circular_buffer.CircularBuffer({length: 5});
+
+ buffer.write({resident:1,GC:2,CC:3});
+ buffer.write({resident:1,GC:3,CC:3});
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+ buffer.clear();
+
+ test.assertEqual(buffer.read(), undefined, "An empty read should be undefined");
+ test.assertEqual(buffer.length, 5, "Buffer's size should be restored");
whimboo
whimboo Mar 23, 2012 Contributor

What do you mean with restored? The length doesn't get changed with write actions.

dglol
dglol Mar 23, 2012 Contributor

I suppose this could be reworded as: "Buffer's length should stay the same".

whimboo
whimboo Mar 23, 2012 Contributor

I would use "... has not been changed"

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+
+ buffer.write({resident:1,GC:1,CC:3});
+ buffer.write({resident:1,GC:2,CC:3});
+ buffer.write({resident:1,GC:3,CC:3});
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ buffer.length = 4;
+
+ test.assertEqual(buffer.shift().GC, 3, "GC field in shifted data should be 3");
+ test.assertEqual(buffer.shift().GC, 4, "GC field in shifted data should be 4");
+ test.assertEqual(buffer.shift().GC, 5, "GC field in shifted data should be 5");
+ test.assertEqual(buffer.shift().GC, 6, "GC field in shifted data should be 6");
+
+ test.assert(buffer.isEmpty(), "Buffer should be empty");
whimboo
whimboo Mar 23, 2012 Contributor

I wouldn't shift entries here and test for an empty buffer because this test is about resizing. Testing the new length would be important.

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ buffer.write({resident:1,GC:2,CC:3});
+ buffer.write({resident:1,GC:3,CC:3});
+ buffer.write({resident:1,GC:4,CC:3});
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ buffer.length = 8;
+
+ test.assertEqual(buffer.shift().GC, 1, "GC field in shifted data should be 1");
+ test.assertEqual(buffer.shift().GC, 2, "GC field in shifted data should be 2");
+ test.assertEqual(buffer.shift().GC, 3, "GC field in shifted data should be 3");
+ test.assertEqual(buffer.shift().GC, 4, "GC field in shifted data should be 4");
+ test.assertEqual(buffer.shift().GC, 5, "GC field in shifted data should be 5");
+ test.assertEqual(buffer.shift().GC, 6, "GC field in shifted data should be 6");
+
+ test.assert(!buffer.isFull(), "Buffer should not be full");
whimboo
whimboo Mar 23, 2012 Contributor

do we have a method to retrieve the current capacity of the buffer? A test for that woud be nice here.

dglol
dglol Mar 23, 2012 Contributor

Agreed.

@whimboo whimboo and 1 other commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ buffer.write({resident:1,GC:5,CC:3});
+ buffer.write({resident:1,GC:6,CC:3});
+
+ buffer.length = 8;
+
+ buffer.write({resident:1,GC:7,CC:3});
+ buffer.write({resident:1,GC:8,CC:3});
+
+ test.assertEqual(buffer.shift().GC, 1, "GC field in shifted data should be 1");
+ test.assertEqual(buffer.shift().GC, 2, "GC field in shifted data should be 2");
+ test.assertEqual(buffer.shift().GC, 3, "GC field in shifted data should be 3");
+ test.assertEqual(buffer.shift().GC, 4, "GC field in shifted data should be 4");
+ test.assertEqual(buffer.shift().GC, 5, "GC field in shifted data should be 5");
+ test.assertEqual(buffer.shift().GC, 6, "GC field in shifted data should be 6");
+ test.assertEqual(buffer.shift().GC, 7, "GC field in shifted data should be 7");
+ test.assertEqual(buffer.shift().GC, 8, "GC field in shifted data should be 8");
whimboo
whimboo Mar 23, 2012 Contributor

There is a lot of code duplication. Mind using helper functions with a loop included?

dglol
dglol Mar 23, 2012 Contributor

This is something I want to do as well. I'll make sure it's in the next commit.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/test/test-buffer.js
+ buffer.length = 8;
+
+ buffer.write({resident:1,GC:7,CC:3});
+ buffer.write({resident:1,GC:8,CC:3});
+
+ test.assertEqual(buffer.shift().GC, 1, "GC field in shifted data should be 1");
+ test.assertEqual(buffer.shift().GC, 2, "GC field in shifted data should be 2");
+ test.assertEqual(buffer.shift().GC, 3, "GC field in shifted data should be 3");
+ test.assertEqual(buffer.shift().GC, 4, "GC field in shifted data should be 4");
+ test.assertEqual(buffer.shift().GC, 5, "GC field in shifted data should be 5");
+ test.assertEqual(buffer.shift().GC, 6, "GC field in shifted data should be 6");
+ test.assertEqual(buffer.shift().GC, 7, "GC field in shifted data should be 7");
+ test.assertEqual(buffer.shift().GC, 8, "GC field in shifted data should be 8");
+
+ test.assert(!buffer.isFull(), "Buffer should not be full");
+}
whimboo
whimboo Mar 23, 2012 Contributor

nit: new line please.

@whimboo whimboo commented on an outdated diff Mar 23, 2012
extension/lib/circular-buffer.js
+ if (typeof(DATA) === 'undefined') {
+ return undefined;
+ }
+
+ if (!this.isEmpty()) {
+ this._count -= 1;
+ }
+
+ this._buffer[this._front] = undefined;
+ this._front = this._nextIndex(this._front);
+ this._emit(ON_REMOVE);
+
+ return DATA;
+ },
+
+ // Writes data to the back of the buffer, same as write(data)
whimboo
whimboo Mar 23, 2012 Contributor

Mind getting rid of write?

Contributor
whimboo commented Mar 23, 2012

Great work so far @dglol. Can't await to get this checked-in.

Contributor
dglol commented Mar 23, 2012

Thanks for the review. Got most of your concerns fixed in the recent commit as well as new tests for the events.

@whimboo whimboo commented on an outdated diff Mar 26, 2012
extension/lib/circular-buffer.js
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+
+const ON_WRITE = 'buffer_write';
+const ON_REMOVE = 'buffer_remove';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(aOptions) {
+ var options = aOptions || {};
+
+ this._length = options.length || 60;
+ if (this._length < 1) {
whimboo
whimboo Mar 26, 2012 Contributor

If aOptions.length was 0 this._length is now 60. I don't think that 0 is even a legal parameter.

@whimboo whimboo and 1 other commented on an outdated diff Mar 26, 2012
extension/lib/circular-buffer.js
+const ON_WRITE = 'buffer_write';
+const ON_REMOVE = 'buffer_remove';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(aOptions) {
+ var options = aOptions || {};
+
+ this._length = options.length || 60;
+ if (this._length < 1) {
+ throw new RangeError('Invalid length');
+ }
+
+ this._front = 0;
+ this._back = 0;
+ this._buffer = [];
+ this._buffer.length = this._length;
whimboo
whimboo Mar 26, 2012 Contributor

Do we really need this._length or can we get routed everything through this._buffer?

dglol
dglol Mar 26, 2012 Contributor

Everything can use this._buffer.length with clear requiring a temporary variable to store the old length.

whimboo
whimboo Mar 26, 2012 Contributor

Which methods would need a local variable for storing the old length? It would be an improvement and kill one dependency when you do not know which variable really has the current length.

@whimboo whimboo commented on an outdated diff Mar 26, 2012
extension/lib/circular-buffer.js
+ }
+
+ this._front = 0;
+ this._back = 0;
+ this._buffer = [];
+ this._buffer.length = this._length;
+ this._count = 0;
+
+ // Report unhandled errors from listeners
+ this.on('error', console.exception.bind(console));
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+ },
+
+ unload: function CircularBuffer_unload() {
whimboo
whimboo Mar 26, 2012 Contributor

Mind adding docs to all methods we have on this class?

@whimboo whimboo and 1 other commented on an outdated diff Mar 26, 2012
extension/lib/circular-buffer.js
+ this.on('error', console.exception.bind(console));
+
+ // Make sure we clean-up correctly
+ unload.ensure(this, 'unload');
+ },
+
+ unload: function CircularBuffer_unload() {
+ this._removeAllListeners(ON_WRITE);
+ this._removeAllListeners(ON_REMOVE);
+ },
+
+ /**
+ * Returns the next index, adjusted for cycles
+ */
+ _nextIndex: function CircularBuffer_nextIndex(aIndex) {
+ return (aIndex + 1) % this._length;
whimboo
whimboo Mar 26, 2012 Contributor

So how is it handled if a client is walking through the indexes but e.g. the memory reporter is pushing other values in the buffer? The index referenced by the client would be invalid because it has been moved one item back. Shouldn't we better use an enumerator to retrieve entries from the buffer? We should really bound the entries to a specific state of the buffer.

dglol
dglol Mar 26, 2012 Contributor

Hmm. Well I wouldn't want consumers to use nextIndex but this is an interesting point. I would prefer to return an array with all the current elements in order and have the consumer traverse that since I'm not sure if there's an in-place method of handling this.

whimboo
whimboo Mar 26, 2012 Contributor

Could be a large array then even when the consumer only needs 2 values from the back of the buffer. A range could probably help here. Would you mind asking some JS devs or Benoit Girard who is working on the internal Profiler which also has a circular buffer. See bug 713227 ff.

@whimboo whimboo commented on an outdated diff Mar 26, 2012
extension/lib/circular-buffer.js
+ if (index < 0) {
+ index += this._length;
+ }
+ return index;
+ },
+
+ get count() {
+ return this._count;
+ },
+
+ get length() {
+ return this._length;
+ },
+
+ set length(aNewLength) {
+ if (aNewLength < 0) {
whimboo
whimboo Mar 26, 2012 Contributor

0 is valid here? It's different to the constructor. :)

@whimboo whimboo commented on an outdated diff Mar 26, 2012
extension/test/test-buffer.js
@@ -0,0 +1,242 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+* License, v. 2.0. If a copy of the MPL was not distributed with this
+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { CircularBuffer } = require("circular-buffer");
+const TestData = [];
+
+// Populate test data
+for (var i = 0; i < 10; i += 1) {
+ TestData.push({ test: i });
+}
+exports.test_state = function (test) {
whimboo
whimboo Mar 26, 2012 Contributor

nit: blank line please

Contributor
whimboo commented Mar 26, 2012

Nice additions. Something which currently strikes me is how we correctly retrieve the data without making it inconsistent. Also I would still like to see it implemented in memchaser's main.js.

Contributor
dglol commented Mar 27, 2012

New commit up.
After experimenting with ways to retrieve the data for iteration, I found that returning a chunk of data was still the best way. The slice method works in the same way as Array.prototype.slice .

@whimboo whimboo commented on the diff Mar 28, 2012
extension/lib/circular-buffer.js
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+'use strict';
+
+const { EventEmitter } = require('api-utils/events');
+const unload = require('api-utils/unload');
+
+const ON_WRITE = 'buffer_write';
+const ON_REMOVE = 'buffer_remove';
+
+var buffer = EventEmitter.compose({
+ constructor: function CircularBuffer(aOptions) {
+ var options = aOptions || {};
+
+ this._buffer = [];
+ this._buffer.length = aOptions.length || 60;
whimboo
whimboo Mar 28, 2012 Contributor

No validation check anymore for negative values?

dglol
dglol Mar 28, 2012 Contributor

It's now implicit, and handled by Array.prototype.length

whimboo
whimboo Mar 28, 2012 Contributor

How is it handled? Do you have some docs?

whimboo
whimboo Mar 29, 2012 Contributor

Got it. Thanks.

@whimboo whimboo and 1 other commented on an outdated diff Mar 28, 2012
extension/lib/circular-buffer.js
+ * NOTE: The objects are copied by reference!
+ */
+ slice: function CircularBuffer_slice(begin, end) {
+ this._normalize();
+
+ return Array.prototype.slice.apply(this._buffer, arguments);
+ },
+
+ /**
+ * Clears data from the buffer and resets the count
+ */
+ clear: function CircularBuffer_clear() {
+ var temp = this.length;
+
+ this._buffer.length = 0;
+ this._buffer.length = temp;
whimboo
whimboo Mar 28, 2012 Contributor

Why not: this._buffer = new Array(this.length)

dglol
dglol Mar 28, 2012 Contributor

Great suggestion. Thanks

@whimboo whimboo and 1 other commented on an outdated diff Mar 28, 2012
extension/lib/circular-buffer.js
+
+ set length(aNewLength) {
+ if (aNewLength < 0) {
+ throw new RangeError('Invalid length');
+ }
+
+ this._normalize();
+
+ // Truncate the oldest elements if new length is less than original
+ if (aNewLength < this.length) {
+ this._buffer = this._buffer.slice(this.length - aNewLength);
+ }
+
+ this._buffer.length = aNewLength;
+ this._count = Math.min(this._count, aNewLength);
+ this._back = this._nextIndex(this._count - 1);
whimboo
whimboo Mar 28, 2012 Contributor

Shouldn't we also use this.count everywhere? If the implementation changes we would have to update a lot of instances.

dglol
dglol Mar 28, 2012 Contributor

Hmm, not sure I understand. What concern do you have?

whimboo
whimboo Mar 28, 2012 Contributor

We have a getter called 'count'. We should always make sure to use it even for the class' own methods.

dglol
dglol Mar 28, 2012 Contributor

Oops, you're right. I had intended to change this along with the this.lengths but it slipped my mind. Thanks

@whimboo whimboo commented on an outdated diff Mar 28, 2012
extension/test/test-buffer.js
+
+ array = buffer.slice(-2);
+
+ test.assertEqual(buffer.length, 10,
+ "Buffer's length should be unchanged after normalizing");
+ test.assertEqual(array.length, 2,
+ "Returned array should have a length of 2");
+ test.assertEqual(array[0].test, buffer.read(-2).test);
+ test.assertEqual(array[1].test, buffer.read(-1).test);
+
+ array = buffer.slice(3, 7);
+
+ for (var i = 0; i < array.length; i += 1) {
+ test.assertEqual(array[i].test, buffer.read(i + 3).test);
+ }
+}
whimboo
whimboo Mar 28, 2012 Contributor

nit: missing new line.

Contributor
whimboo commented Mar 28, 2012

I have made some more thoughts on how to retrieve the data without inconsistencies. In C++ we would have a pointer but that doesn't work in Javascript. So could we use an unique id for each element in the buffer, which gets counted up? That way we could use the slice method to start by that id (if specified) and return new entries only. It would enable us to log data in chunks.

Contributor
dglol commented Mar 28, 2012

I have two suggestions too, and I don't think it should belong on the circular buffer.

  1. We can also have the logger have a counter that keeps track of the number of writes and always slice off a statically-indexed part of the buffer. The only problem are front/queue-style operations, which we can fix by also tracking the number of queue operations or removing them completely (front operations are not very useful for data streams).
  2. We can have the logger keep a static sized queue that flushes once it hits a limit.
Contributor
whimboo commented Mar 28, 2012

I like option 1 because any consumer would be able to handle data on its own. Means directly or in chunks. Would be good to see if we could move as much code as possible to the CircularBuffer implementation.

Contributor
dglol commented Mar 29, 2012

New commit up with only the refactors but not the solution to retrieve data without inconsistencies.
I have a feeling a lot of logic needs to be implemented for option 1 even though it does seem to be a good solution. It might be easier to implement if we remove unshift though. Besides, pushing data to the front of a buffer seems to be an unnecessary feature.

Contributor
whimboo commented Mar 30, 2012

I agree on that statement. It's probably right that we will never have operations at the front of the buffer. So go for it.

@whimboo whimboo commented on an outdated diff Mar 30, 2012
extension/lib/circular-buffer.js
+ },
+
+ set length(aNewLength) {
+ if (aNewLength < 0) {
+ throw new RangeError('Invalid length');
+ }
+
+ this._normalize();
+
+ // Truncate the oldest elements if new length is less than original
+ if (aNewLength < this.length) {
+ this._buffer = this._buffer.slice(this.length - aNewLength);
+ }
+
+ this._buffer.length = aNewLength;
+ this._count = Math.min(this.count, aNewLength);
whimboo
whimboo Mar 30, 2012 Contributor

Do we also want to call the count setter here? Also applies to other lines of code.

Contributor
whimboo commented Mar 30, 2012

This will not make 0.3. Moving to 0.4.

@whimboo whimboo removed this from the 0.6 milestone Feb 14, 2014
Contributor
whimboo commented Oct 24, 2014

This PR hasn't been updated for a long time. If we want to continue I would propose to create a new PR.

@whimboo whimboo closed this Oct 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment