INT-203 arrays

[rueckstiess]

WIP, just making pull request to discuss some details. Don't merge yet!

[rueckstiess]

Review status: 0 of 16 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

^{lib/field-collection.js, line 11 [r1] (raw file):}
I find these classNames very helpful for debugging.

---

^{lib/field-collection.js, line 31 [r1] (raw file):}
this was in parser before. With the restructuring, it made sense to add it here and to TypeCollection.

---

^{lib/field-collection.js, line 41 [r1] (raw file):}
we have missed some "Undefined" values for this key, see comment in ./type.js below.

---

^{lib/field.js, line 132 [r1] (raw file):}
All of these listeners would be great if we tried to keep the whole schema consistent even if you manipulate children collections, like remove a value. But we already assume that we only ever insert into schema, e.g. the .count variables go always only up. Therefore I suggest we remove all of this and just insert the value to the Field#values collection manually before passing it down to the type.

---

^{lib/field.js, line 197 [r1] (raw file):}
I'm trying hard to avoid this commit step for performance reasons. Calculating the unique numbers after every single document is O(n), sorting a children collection O(n log n). Doing it after every doc seems excessive, given the usual use case to parse 100 docs before doing anything. I'd rather listen to an 'end' event after a batch was parsed, and then calculate once. We're not accessing unique during the streaming?

The probabilities are already taken care of, they are now derived properties basically just this.count / this.parent.count (with some edge cases for undefined and arrays).

unique and sorted types are still missing.

---

^{lib/schema.js, line 8 [r1] (raw file):}
I thought this was quite clever :-) makes the code so much more elegant.

---

^{lib/schema.js, line 25 [r1] (raw file):}
we could listen to this 'end' event on schema to trigger the "cleanup" tasks like calculating unique values, sorting collections, etc.

---

^{lib/type-collection.js, line 37 [r1] (raw file):}
does this work? I have a feeling it won't for browserify. Been going crazy over these circular imports...

---

^{lib/type.js, line 232 [r1] (raw file):}
this is a nice way to handle the Undefined values. It iterates over the union of object keys and existing field names, automatically inserting undefined for the fields that are not in the current object.

Only caveat is that when a field is created for the first time, we need retrospectively bump up the undefined count. That's taken care of in ./field-collection.js.

---

Comments from the review on Reviewable.io

[rueckstiess]

Review status: 0 of 16 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

^{lib/field.js, line 132 [r1] (raw file):}
on second thought: should we just remove Field#values altogether? We're not using it in the UI (the unique minichart across all types was a hack for the demo that Dan wanted). Cuts the size of the schema in half if we get rid of it here. And it's pretty easy to get all values of all types with some lodash.

---

Comments from the review on Reviewable.io

[imlucas]

Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

^{lib/field-collection.js, line 11 [r1] (raw file):}
Nice! Perhaps use namespace instead? https://github.com/ampersandjs/ampersand-state

Confused className w/ Backbone's View className http://backbonejs.org/#View-extend

---

^{lib/field-collection.js, line 31 [r1] (raw file):}
Awesome!

---

^{lib/field-collection.js, line 41 [r1] (raw file):}
Not sure what comment you're referring to in type :/ Add an @see perhaps? http://usejsdoc.org/tags-see.html

---

^{lib/field.js, line 10 [r1] (raw file):}
see above on namespace

---

^{lib/field.js, line 132 [r1] (raw file):}
Yes, just remove Field#values altogether. Another demo proofing hack.

---

^{lib/field.js, line 197 [r1] (raw file):}
yep all code to quickly ensure demo stability. thrilled to see it die.

---

^{lib/schema.js, line 8 [r1] (raw file):}
yep this is great! I've been wanting this for a while.

---

^{lib/schema.js, line 25 [r1] (raw file):}
yep. per perf comment above on removing Field#commit, we should strive to figure out getting ampersand configs right on Collections and States so we dont need cleanup.

---

^{lib/type-collection.js, line 37 [r1] (raw file):}
Yes this won't work. Perhaps move ./type.js#Type to /type-base.js or something?

---

^{lib/type.js, line 31 [r1] (raw file):}
If you declare parent this will work correctly :) Before derived: {, add session: {parent: 'state'}, derived: {

---

^{lib/type.js, line 47 [r1] (raw file):}
Types which do not need to store values.

---

^{lib/type.js, line 67 [r1] (raw file):}
Perhaps a class level comment would be helpful here because of the small cabal that actually knows all of the bson types by heart :)

/**
 * @see http://mongodb.github.io/node-mongodb-native/2.0/api/MaxKey.html
 */

---

^{lib/type.js, line 75 [r1] (raw file):}

/**
 * @see http://mongodb.github.io/node-mongodb-native/2.0/api/MinKey.html
 */

---

^{lib/type.js, line 123 [r1] (raw file):}
More BSON @see's needed http://mongodb.github.io/node-mongodb-native/2.0/api/Double.html

---

^{lib/type.js, line 232 [r1] (raw file):}
Add that caveat to the docs in the code

---

^{lib/type.js, line 244 [r1] (raw file):}
#types child collection is a great idea

---

^{lib/type.js, line 246 [r1] (raw file):}
per above on circular imports, you could also solve this by making type a folder and breaking out each of these class groups into their own files under type (eg /type/array.js, /type/primitive.js) and then stitch together in /types/index.js using _.extend(module.exports, require(./array'), require('./primitive')); etc

---

Comments from the review on Reviewable.io

[rueckstiess]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

^{lib/field-collection.js, line 11 [r1] (raw file):}
namespace works. didn't get what its exact purpose is from the docs, but seems optional anyway. Changing to namespace.

---

^{lib/field-collection.js, line 41 [r1] (raw file):}
Sorry, was referring to the comment in this code review below for ./type.js, that starts with

this is a nice way to handle the Undefined values.

I'll add comments to the code in both files to make it clear.

---

^{lib/field.js, line 197 [r1] (raw file):}
👍

+enhancement +@rueckstiess remove Field#commit and fix unique and sorting

---

^{lib/schema.js, line 25 [r1] (raw file):}
Yep!

---

^{lib/type-collection.js, line 37 [r1] (raw file):}
will follow your advise below to break types into individual files in a types folder. Think that's cleaner anyway.

+bug +@rueckstiess don't use circular imports.

---

^{lib/type.js, line 246 [r1] (raw file):}
I'll do that.

+enhancement +@rueckstiess break types into individual files under ./types

---

Comments from the review on Reviewable.io

[rueckstiess]

Reviewed 25 of 25 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

^{lib/field.js, line 37 [r1] (raw file):}
since we don't have values at the Field level anymore, we can make unique a derived property, see below.

---

^{lib/field.js, line 91 [r2] (raw file):}
sums each type's unique number to get the unique number for the field level. I've made this cache: false and not dependent on anything, so that it does not get computed on every insert, but only when requested (which is only once in the UI, not 100x).

---

^{lib/field.js, line 103 [r2] (raw file):}
same here, because it uses this.unique.

---

^{lib/index.js, line 18 [r2] (raw file):}
when is this the case? is this a stream convention? only found the .pipe below. I left both.

---

^{lib/type-collection.js, line 32 [r2] (raw file):}
same as unique: don't sort the collection 100 times during parsing, only sort it once on request (either via .serialize() or explicitly, in scout)

---

^{test/events.test.js, line 19 [r2] (raw file):}
after removing all the collection event handlers, this was obviously not possible anymore. but is it really necessary?

---

Comments from the review on Reviewable.io

[imlucas]

Reviewed 2 of 25 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{lib/field.js, line 88 [r2] (raw file):}
copy+paste error. This is Type#unique

---

^{lib/field.js, line 91 [r2] (raw file):}
Add I've made this cache: false and not dependent on anything, so that it does not get computed on every insert. to docs.

---

^{lib/index.js, line 3 [r2] (raw file):}
Unused?

---

^{lib/index.js, line 18 [r2] (raw file):}
Apologies. Poopy docs on my part. This case is for if you pass a Cursor instance.

---

Comments from the review on Reviewable.io

[imlucas]

Reviewed 23 of 25 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{lib/type-collection.js, line 32 [r2] (raw file):}
Also part of the "configuring ampersand correctly" discussion as it should just handle this for us and we just implement the comparator.

---

^{test/events.test.js, line 19 [r2] (raw file):}
Good to remove! Mostly this was just a case we were trying to debug for demo.

---

Comments from the review on Reviewable.io

[imlucas]

Reviewed 15 of 15 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[imlucas]

Reviewed 2 of 10 files at r4.
Review status: 25 of 33 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io