Add starring

[ihodes]

Fixes https://github.com/hammerlab/cycledash/issues/675

WIP: fix tests & add tests for new feature.

[jaclynperrone]

Oooo. Excited for this one!

[danvk]

This mostly looks fine (thanks for implementing it!), but before we get too into the review, do you know why the fonts changed on the screenshots? e.g. in examine_bad-query.png the "Compare to..." text takes up much more of the box after this change.

[ihodes]

Yeah I noticed that, too; the tests were (incorrectly) using the TypeKit engine for a few commits; this reverts it to the non-Typekit fonts we've been using for a while. (We don't want people to need a Typekit account in order to run the screenshot tests).

[danvk]

Reviewed 6 of 9 files at r1, 4 of 24 files at r2. Review status: 10 of 31 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

^{cycledash/static/js/examine/RecordStore.js, line 274 [r2] (raw file):} foo and realFoo or actualFoo is a minor pet peeve for me. realRecord makes me question my understanding of what record was in the first place. It's more of an issue in languages where you don't have to declare the types of each variable.

How about names which indicate something more about what they are? e.g. recordFromStore. A comment would be helpful, too. I honestly don't know what's going on here.

---

^{cycledash/static/js/examine/RecordStore.js, line 333 [r2] (raw file):} Also not sure how I feel about the naming here... just looking at the names, it seems clear what the difference between starGenotype and starGenotypeAndNotify should be. But that's not at all what the distinction between the functions is. Not sure what a better name would be, though. Maybe setGenotypeStarAndNotify?

---

^{tests/python/test_genotypes_api.py, line 48 [r2] (raw file):} how about a test where it fails, too? (e.g. because of an invalid contig)

---

^{workers/shared.py, line 157 [r2] (raw file):} This seems strange — if a column has TRUE/FALSE values set, isn't it "extant"?

---

Comments from the review on Reviewable.io

[danvk]

(LGTM, feel free to merge once you've addressed my comments & the tests pass.)

---

Review status: 10 of 31 files reviewed at latest revision, 4 unresolved discussions, some commit checks broke.

---

Comments from the review on Reviewable.io

[ihodes]

Review status: 7 of 31 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

^{workers/shared.py, line 157 [r2] (raw file):} Yeah; this wasn't the right way to do it. What I explicitly want is a way to ignore certain columns. Starred is special in that I don't want it in the list of columns the VCFTable handles the display of (otherwise the it'd be displayed very differently); I want to handle its display myself. I added an EXCLUDED_FROM_EXTANT_COLUMNS variable to track columns I don't want to end up there.

---

Comments from the review on Reviewable.io

[ihodes]

Thanks for the review! Let's see if Travis doesn't time out again (the tests look like they've been hanging on Travis… hopefully this got resolved overnight…)