Ampersand-ify

[imlucas]

Loads of cleanup post mongodb world. Ready to get this back to master to get contributions/feedback.

[rueckstiess]

Main Feedback

1. Beautiful commenting ❤️
2. Type names: consider lowercase (see comment at end of ./lib/type.js)
3. README example doesn't match actual output, needs update
4. Field idAttribute: consider using name instead
5. Type idAttribute: consider using name instead
6. Arrays: have a proposal how to handle in terms of schema, but out of scope for here. I'll comment on INT-203.

More details below.

---

Reviewed 1 of 12 files at r1, 3 of 7 files at r2, 18 of 34 files at r4, 22 of 22 files at r6.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

^{docs/mongodb-schema_diagram.md, line 24 [r6] (raw file):}
lengths is array of ints, not int.

---

^{lib/field.js, line 9 [r6] (raw file):}
How about

/**
 * Field describes the properties of a single field (e.g. "_id") and its sampled values.
 */

---

^{lib/field.js, line 16 [r6] (raw file):}
as mentioned in ./README.md, would be better to call this name and change idAttribute to name.

Also need to double-check that all code uses field.getId() instead of field._id everywhere.

---

^{lib/field.js, line 50 [r6] (raw file):}
if renamed or not, this should be this.getId()

---

^{lib/field.js, line 70 [r6] (raw file):}
Replace with

* Type of the values. String for single type, array of strings for multiple types.

---

^{lib/field.js, line 89 [r6] (raw file):}
I don't think I understand what total is. Can you give an example for parentIsArray case true and false?

---

^{lib/field.js, line 93 [r6] (raw file):}
would be nicer to ask directly, e.g.

var parentIsArray = this.collection.parent.type === 'Array';

---

<sup>**[lib/field.js, line 136 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTjm9OaIekv3lt0SrF)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/field.js#L136)):</sup>
typo `reflected`

---

<sup>**[lib/field.js, line 209 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTkPK3zm2-Q9M3ytjw)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/field.js#L209)):</sup>
remove `)`

---

<sup>**[lib/field.js, line 219 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTkrXsEPJjp4r6RK53)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/field.js#L219)):</sup>
hm, is this `(undef.count / this.count)` ?

Wonder why I'm getting the right result though. Perhaps updated somewhere else.

---

<sup>**[lib/field.js, line 222 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTlh3Y726dBhXYBKho)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/field.js#L222)):</sup>
ah yes, suppose this will overwrite the incorrect result above.

---

<sup>**[lib/type.js, line 8 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsToetGZdSbScY2Pq30)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/type.js#L8)):</sup>
would also rename `_id` to `name`, or here just `id`, but I see no need to conform to MongoDB naming convention here. It's not a document.

---

<sup>**[lib/type.js, line 158 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTpOjmz_orDpJcWvkK)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/lib/type.js#L158)):</sup>
We seem to be loosely going by "json-schema" conventions. They use lower-case [names for the types]( http://spacetelescope.github.io/understanding-json-schema/reference/type.html). Javascript also uses lower-case names for types. Any reason to go uppercase?

---

<sup>**[README.md, line 11 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTT5t-1FtQcbUAiXDq)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L11)):</sup>
remove second "so".

---

<sup>**[README.md, line 14 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTZifYE767O91UUJ1E)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L14)):</sup>
> `npm install mongodb mongodb-schema`

for clarity and consistency (`npm install` used below)

---

<sup>**[README.md, line 16 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTUzwWrC39kPO10NrD)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L16)):</sup>
More clear: 

> Create a new file `parse-schema.js` and paste in the following code: 

---

<sup>**[README.md, line 37 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTUu2s2kiuP3KfPVor)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L37)):</sup>
These "comment" keys are weird and confusing. Instead, I would just put the comments on the same line after the field (use `js` instead of `json` for code fence to avoid red comments) , and modify the sentence at the top to:

> ..., we'll see output similar to this (without the comments): 

---

<sup>**[README.md, line 39 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTcG41sYywuH-XBISD)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L39)):</sup>
`"ns": "test.test",` missing, but present in the actual output.

---

<sup>**[README.md, line 43 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTcSxtsdc_RYHBraTY)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L43)):</sup>
`_id` as the label for the name of the field is confusing. (MongoDB calling its main key `_id` is not the same.)

Suggest renaming to `name` and setting `Field.idAttribute` to `name`.

---

<sup>**[README.md, line 45 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTXd0S5HcZJvGkUyg1)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L45)):</sup>
`count` is missing in fields, but it's present in the actual output.

---

<sup>**[README.md, line 68 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTZJgd1W1O1Se8ozef)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L68)):</sup>
`count` missing in types, too, put present in the actual output. 

---

<sup>**[README.md, line 84 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTZzGd2upwKM10XrtT)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L84)):</sup>
how is `unique` defined inside `Undefined` pseudo-type? I'd expect this to be `1`, as it is only 1 document that is undefined, hence unique. I'd imagine it should work like type `null`.

If we don't support `unique` inside `Undefined`, maybe remove to avoid confusion?

---

<sup>**[README.md, line 91 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTdPqu_qwbclVpuEGf)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L91)):</sup>
In addition to `count`, also missing in this example (but present in actual output): 

  "type": "Number",
  "total": 4,
  "has_duplicates": false,
  "values": [
    2,
    3,
    4,
    1
  ],

Need documentation. 

---

<sup>**[README.md, line 94 \[r6\]](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19#-JsTZV3GZnarHx3OL84v)** ([raw file](https://github.com/mongodb-js/mongodb-schema/blob/1e4b5238cff246b54cc838b11667e4602541cb97/README.md#L94)):</sup>
> is quite powerful and 

perhaps remove that part?

---


Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/mongodb-js/mongodb-schema/19)
<!-- Sent from Reviewable.io -->

[imlucas]

Review status: 35 of 41 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{docs/mongodb-schema_diagram.md, line 24 [r6] (raw file):}
fixed

---

^{lib/field.js, line 89 [r6] (raw file):}
updated. this is still kind of a shit hack but will be fixed when we switch to not having subclassing for EmbeddedDocumentField and EmbeddedArrayField per INT-203

---

^{lib/type.js, line 8 [r6] (raw file):}
fixed

---

^{lib/type.js, line 158 [r6] (raw file):}
bson uses titlecase

---

^{README.md, line 45 [r6] (raw file):}
left out for simplicity. need better clarification of count vs. total internally and will add back.

---

^{README.md, line 91 [r6] (raw file):}
again, just left out for simplicity

---

Comments from the review on Reviewable.io

[imlucas]

Review status: 35 of 41 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{lib/type.js, line 158 [r6] (raw file):}
also JS uses titlecase https://github.com/mongodb-js/mongodb-schema/pull/19/files#diff-4e2abe5a12642efc78b26087a61a8e9fR34

---

Comments from the review on Reviewable.io

[imlucas]

Review status: 35 of 41 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{README.md, line 43 [r6] (raw file):}
Done.

---

^{README.md, line 94 [r6] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[rueckstiess]

Reviewed 1 of 2 files at r7, 18 of 18 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io