Integrate with a CSS parser for selector parsing #1086

aweary · 2017-08-21T18:38:47Z

Resolves #694, #693, #547, #1082, #1019, #1078, #1086, #283

This PR integrates with rst-selector-parser a fork of scalpel which is a CSS parser implemented with nearley. This fork implements a custom grammar requiring quoted string attribute values, allowing unquoted numbers, booleans, and other primitives, and supports selectors in any order.

Major Changes

buildPredicate is exported by the selectors module, the API remains the same
ComplexSelector is no longer needed. Instead the selectors module exports a reduceTreeBySelector which manages traversal + matching with simple selectors and combinators. Both the shallow and mount wrappers use the same buildPredicate and findWhereUnwrapped API so there's no need to have a class for selector parsing.

Pending Changes

These are changes that I still need to make before this can be merged.

Remove ComplexSelector and all utility methods that were only used by ComplexSelector
Throw if a complex selector is passed to buildPredicate
Handle PSEUDO_CLASS and PSEUDO_ELEMENT selectors
Add tests for valid selectors that were previously unparsable

aweary · 2017-08-21T21:04:27Z

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx

@@ -433,22 +435,15 @@ describeWithDOM('mount', () => {
      expect(wrapper.find('.row + .row')).to.have.lengthOf(1);
    });

-    // React 15.2 warns when setting a non valid prop to an DOM element
-    describeIf(REACT013 || REACT014, 'unauthorized dom props', () => {


This test was kind of a false positive. You could still query unauthorized DOM props by using the object syntax, and AFAICT it was only failing to find them because of how it was parsing the selector.

React <0.14 didn't just ignore syntactically invalid attributes, it ignored attributes that it didn't have whitelisted. To implement this behavior as it should be implemented we'd need to integrate that same whitelist.

Can we add that whitelist in the react 0.14 and 0.13 adapters?

Sure, OK to do in a follow up?

Yes, totes ok to do in a followup

ljharb

Pausing my review pending answers about the quoted string question.

ljharb · 2017-08-21T22:24:50Z

packages/enzyme/src/Utils.js

@@ -308,14 +308,6 @@ export function coercePropValue(propName, propValue) {

  const trimmedValue = propValue.trim();

-  // if propValue includes quotes, it should be


Why? The fact that selectors can omit quotes is imo very problematic; enforcing strings ensures that selectors used will be unambiguous.

ljharb · 2017-08-21T22:25:50Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

@@ -155,13 +153,6 @@ describe('RSTTraversal', () => {
      expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', '-Infinity')).to.equal(true);
      expect(nodeHasProperty(<div foo={0} />, 'foo', '-Infinity')).to.equal(false);
    });
-
-    it('should throw when un unquoted string is passed in', () => {


tagging this line too - i think the purpose of requiring strings here was so people would distinguish between numbers and strings, for example.

Regardless of what mistakes React 16 has made, older Reacts do, and non-Reacts might, still want to distinguish these on host components - and they'll always need distinguishing on custom components.

According to the spec, unquoted values must be valid identifiers or strings. The selectors [foo=bar] and [foo="bar"] are thus parsed identically. A spec-compliant parser would return identical tokens for both of those selectors, and that's indeed what scalpel does.

Using numbers as the attribute value is a violation of the spec, e.g., [foo=42], since valid CSS identifiers cannot start with numbers. Implementing this behavior would require using a selector parser that does not conform to the selector spec.

Right - the problem is that we can't use spec-compliant selectors, because we're not selecting HTML - we're selecting from an RST, which has non-string values (HTML only has strings).

In other words, we can only use the actual selector spec if we're restricting selectors to DOM elements - so enzyme's selection is always going to have to be different than the web spec (we test react native too, for example)

In that case, I can just fork scalpel and redefine the grammar to require unquoted values be numerical. I'm not super familiar with writing parser grammars but I think I can do it 😄

it'd be even better if we could make a new package that avoids forking and instead directly depends on scalpel, but i'm not sure how hard that'd be :-/

It wouldn't really be possible, this requires making a change directly to the grammar for attribute values and there's no way to do that without changing grammar.ne.

aweary · 2017-08-21T23:24:47Z

@ljharb here's a PR to my fork of scalpel that updates the grammar to require quoted string values and also parses unquoted numerical values: aweary/rst-selector-parser#1

I was going to publish this as rst-selector-parser and use that in this PR instead.

aweary · 2017-08-22T15:44:50Z

@ljharb I've integrated the new rst-selector-parser package which has actually simplified things. I updated the grammar to parse all the unquoted values we support (aweary/rst-selector-parser#2, aweary/rst-selector-parser#3, aweary/rst-selector-parser#4) so the parser will return the value we want instead of a stringified version (for example, [foo=NaN] will return NaN for the token value not "NaN").
This means we no longer need to explicitly coerce prop values.

aweary · 2017-08-22T15:46:02Z

packages/enzyme/src/Utils.js

-  throw new TypeError(
-    `Enzyme::Unable to parse selector '[${propName}=${propValue}]'. ` +
-    `Perhaps you forgot to escape a string? Try '[${propName}="${trimmedValue}"]' instead.`,
-  );


Any selector that is unparsable will fail during the parsing step now.

aweary · 2017-08-23T18:37:55Z

@ljharb @lelandrichardson I think this is ready for a full review. I marked all the issues this should resolve in the top comment and added tests for them.

aweary · 2017-08-23T18:40:09Z

packages/enzyme/src/selectors.js

+      return nodeHasProperty(node, token.name, token.value);
+    case PSEUDO_ELEMENT:
+    case PSEUDO_CLASS:
+      throw new Error('Enzyme::Selector does not support psuedo-element or psuedo-class selectors.');


Adding support for pseudo-selectors will be really easy. For example, implementing :not (#460). I figure this can be done in a follow-up.

For reference, I implemented :not with this same approach here: https://github.com/aweary/react-tester/blob/master/src/selector-parser.js#L91-L102

Followups for these things are great; this PR's big enough already :-)

aweary · 2017-08-25T17:26:59Z

packages/enzyme/src/selectors.js

+     * @example '[data-foo=foo]' matches <div data-foo="foo" />
+     */
+    case ATTRIBUTE_VALUE:
+      return nodeHasProperty(node, token.name, token.value);


I just realized the parser returns the operator used in attribute value selectors, so we can now support selectors like [foo|="bar"] and [foo^="bar"]

aweary · 2017-08-29T23:02:38Z

@ljharb do you think we'll be able to get this in for v3?

ljharb

Thanks, this is looking great.

Aside from the breaking change of our helper functions, is there anything in the main shallow/mount API that used to work, but will stop working as a result of this change? If so, we'll need to include it in our v3 migration guide.

ljharb · 2017-09-01T07:30:46Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-    });
-
-    it('throws for malformed selectors', () => {
-      expect(() => fn('div[data-name="xyz"')).to.throw(/Enzyme::Selector received what appears to be a malformed string selector/);


does this no longer throw (somewhere)? it's a malformed selector.

It does throw, but since the new rst-selector-parser manages all parsing this should be covered by the unit tests for the package. I figure at this point we'd be testing the behavior of a dependency with its own tests.

But I'll add back a test that ensures that we catch the general parsing error and re-throw something more specific. Then I think it's safe to assume invalid selectors will throw, and if the rst-selector-parser coverage is missing a case we can add it there.

On that note, I'd be happy to include rst-selector-parser as part of this monorepo if we want to.

ljharb · 2017-09-01T07:32:11Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-    it('should parse false as a literal', () => {
-      const node = $(<div foo />);
-
-      expect(nodeHasProperty(node, 'foo', 'true')).to.equal(true);


should this test not still be valid for true instead of 'true'?

It should, I'll add that case.

Added test case for booleans: https://github.com/airbnb/enzyme/pull/1086/files#diff-67b0df7735c607bdef49d8a20292e57aR71

ljharb · 2017-09-01T07:32:45Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-      expect(nodeHasProperty(<div foo={2} />, 'foo', '-0')).to.equal(false);
+      expect(nodeHasProperty(<div foo={0} />, 'foo', 0)).to.equal(true);
+      expect(nodeHasProperty(<div foo={0} />, 'foo', +0)).to.equal(true);
+      expect(nodeHasProperty(<div foo={-0} />, 'foo', -0)).to.equal(true);


can we add test cases that -0 does not match 0, and 0 does not match -0?

ljharb · 2017-09-01T07:33:02Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

+      expect(nodeHasProperty(<div foo={0} />, 'foo', +0)).to.equal(true);
+      expect(nodeHasProperty(<div foo={-0} />, 'foo', -0)).to.equal(true);
+      expect(nodeHasProperty(<div foo={1} />, 'foo', 0)).to.equal(false);
+      expect(nodeHasProperty(<div foo={2} />, 'foo', -0)).to.equal(false);
    });

    it('should work with empty strings', () => {
      expect(nodeHasProperty(<div foo={''} />, 'foo', '')).to.equal(true);


for funsies, let's add foo="" also?

ljharb · 2017-09-01T07:33:43Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-
-      expect(() => nodeHasProperty(node, 'title', 'foo')).to.throw();
+      expect(nodeHasProperty(<div foo={Infinity} />, 'foo', Infinity)).to.equal(true);
+      expect(nodeHasProperty(<div foo={Infinity} />, 'foo', +Infinity)).to.equal(true);


let's add a case where Infinity fails to match a prop value of Infinity?

It'd also be good to ensure NaN and Infinity don't equate, since both are not finite

ljharb · 2017-09-01T07:37:40Z

packages/enzyme/src/selectors.js

+ * @param {Token} token 
+ */
+function nodeMatchesToken(node, token) {
+  if (node === null || typeof node === 'string') {


can you elaborate on why a string node wouldn't match? is there no selector that can match text?

Correct, with the current grammar there's no way to match a string literal. foo would match an element of type foo. We could augment the grammar to allow something like "foo" but I'm not sure how useful that really is.

ljharb · 2017-09-01T07:38:07Z

packages/enzyme/src/selectors.js

+      return nodeHasProperty(node, token.name, token.value);
+    case PSEUDO_ELEMENT:
+    case PSEUDO_CLASS:
+      throw new Error('Enzyme::Selector does not support psuedo-element or psuedo-class selectors.');


Followups for these things are great; this PR's big enough already :-)

ljharb · 2017-09-01T07:38:37Z

packages/enzyme/src/selectors.js

+ * @param {Function|Object|String} selector 
+ */
+export function buildPredicate(selector) {
+  switch (typeof selector) {


fwiw, this bypasses the engine optimization when typeof foo is repeated. This would be better as an if/else, for a number of reasons.

@ljharb can you elaborate on what optimization this bypasses? I'm not familiar.

engines are able to do typeof foo === 'some string' checks very very fast; when you store typeof foo in a variable (or a binding, as here) it has to go the slower path.

ljharb · 2017-09-01T07:39:23Z

packages/enzyme/src/selectors.js

+      // constructor
+      return node => node && node.type === selector;
+    case 'object':
+      if (!Array.isArray(selector) && selector !== null && !isEmpty(selector)) {


what does lodash return for isEmpty(null)?

if we're keeping !== null for perf, then we should be checking that first, before isArray.

I'll figure out what it returns and update this. I just copied what was already there FWIW: https://github.com/airbnb/enzyme/blob/c15dd479684a9e3423d3efb5aea592fcfde1a842/packages/enzyme/src/RSTTraversal.js#L119

ljharb · 2017-09-01T07:39:47Z

packages/enzyme/src/selectors.js

+export function reduceTreeBySelector(selector, wrapper) {
+  const root = wrapper.getNodeInternal();
+  let results = [];
+  switch (typeof selector) {


same here wrt typeof and if/else

aweary

@ljharb thanks! I'll move those changes today.

I think this should be backward compatible. There may be breaking changes if we were silently incorrectly parsing and applying a selector before, but as far as expected behavior goes it should be consistent.

aweary · 2017-09-01T16:05:13Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-    });
-
-    it('throws for malformed selectors', () => {
-      expect(() => fn('div[data-name="xyz"')).to.throw(/Enzyme::Selector received what appears to be a malformed string selector/);


It does throw, but since the new rst-selector-parser manages all parsing this should be covered by the unit tests for the package. I figure at this point we'd be testing the behavior of a dependency with its own tests.

But I'll add back a test that ensures that we catch the general parsing error and re-throw something more specific. Then I think it's safe to assume invalid selectors will throw, and if the rst-selector-parser coverage is missing a case we can add it there.

On that note, I'd be happy to include rst-selector-parser as part of this monorepo if we want to.

aweary · 2017-09-01T16:05:44Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-    it('should parse false as a literal', () => {
-      const node = $(<div foo />);
-
-      expect(nodeHasProperty(node, 'foo', 'true')).to.equal(true);


It should, I'll add that case.

aweary · 2017-09-01T16:06:33Z

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx

@@ -433,22 +435,15 @@ describeWithDOM('mount', () => {
      expect(wrapper.find('.row + .row')).to.have.lengthOf(1);
    });

-    // React 15.2 warns when setting a non valid prop to an DOM element
-    describeIf(REACT013 || REACT014, 'unauthorized dom props', () => {


Sure, OK to do in a follow up?

aweary · 2017-09-01T16:07:16Z

packages/enzyme/package.json

@@ -39,6 +39,7 @@
    "object.assign": "^4.0.4",
    "object.entries": "^1.0.4",
    "raf": "^3.3.2",
+    "rst-selector-parser": "^2.2.0",
    "uuid": "^3.1.0"


Yes! I'll remove it.

aweary · 2017-09-01T16:10:31Z

packages/enzyme/src/selectors.js

+ * @param {Token} token 
+ */
+function nodeMatchesToken(node, token) {
+  if (node === null || typeof node === 'string') {


Correct, with the current grammar there's no way to match a string literal. foo would match an element of type foo. We could augment the grammar to allow something like "foo" but I'm not sure how useful that really is.

aweary · 2017-09-01T16:11:44Z

packages/enzyme/src/selectors.js

+      // constructor
+      return node => node && node.type === selector;
+    case 'object':
+      if (!Array.isArray(selector) && selector !== null && !isEmpty(selector)) {


I'll figure out what it returns and update this. I just copied what was already there FWIW: https://github.com/airbnb/enzyme/blob/c15dd479684a9e3423d3efb5aea592fcfde1a842/packages/enzyme/src/RSTTraversal.js#L119

aweary · 2017-09-02T00:01:21Z

@ljharb I think I've addressed all of your feedback!

lelandrichardson · 2017-09-10T16:57:19Z

packages/enzyme-test-suite/test/selector-spec.jsx

+      it('simple adjacent', () => {
+        const wrapper = renderMethod(
+          <div>
+            <div className="to-find" />


something worth testing is to have children of a node that are in separate arrays but adjacent to one another:

<div> <div className="a" /> {[<div key="0" className="b" />]} </div>

With this you can assert:

expect(wrapper.find('.a + .b'))..to.have.lengthOf(1)

lelandrichardson · 2017-09-10T16:59:40Z

packages/enzyme/src/RSTTraversal.js

+ * @param {*} root 
+ * @param {*} targetNode 
+ */
+export function findParentNode(root, targetNode) {


just curious.... RSTTraversal.js has a parentsOfNode function that you could potentially use for this? Did it not work when you tried or did you not know about it?

I did see that, and it wasn't working for me. I didn't spend a lot of time trying to fix it though since I wasn't really following how it was implemented. I'm not actually sure how it's working at all, since it calls reverse() on the result of pathToNode, but pathToNode returns null, which is the error I was getting

TypeError: Cannot read property 'reverse' of null at parentsOfNode (packages/enzyme/build/RSTTraversal.js:121:10)

It's also more complicated than we need for selector matching since AFAICT it finds all parents, not just the direct parent. If we add a parent pointer per discussions in #1085 this utility could go away altogether.

Note that that error is a pre-existing bug in enzyme 2 - #410

lelandrichardson · 2017-09-10T17:01:05Z

packages/enzyme/src/selectors.js

+ * by scalpel.
+ * @param {String} selector 
+ */
+function safelyGenerateTokens(selector) {


feels weird to call this safely{thing} but still have it throw

Nah throwing is the safest thing ever! Unsafe things are what don't throw :-)

Yeah, I was thinking "safe" because we explicitly throw our own error instead of letting the parser crash, and know that the tokens don't require additional validation.

lelandrichardson

wow this PR is SOOOO GOOOD. Thank you so much for doing this. Looks like it was a ton of work to do right but this is so much better than what we had. I had a couple of minor comments, but nothing blocking. Once Jordan signs off, let's get this in!

lelandrichardson · 2017-09-10T17:05:50Z

Oh, also, sorry this took me so long to look at!

ljharb

LGTM once the remaining comments have been addressed

ljharb · 2017-09-10T17:47:41Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-      expect(nodeHasProperty(<div foo={null} />, 'foo', 'null')).to.equal(true);
-      expect(nodeHasProperty(<div foo={0} />, 'foo', 'null')).to.equal(false);
+      expect(nodeHasProperty(<div foo={null} />, 'foo', null)).to.equal(true);
+      expect(nodeHasProperty(<div foo={0} />, 'foo', null)).to.equal(false);


Let's also add a pair of assertions that null won't equal undefined?

What's the expected behavior with undefined? It looks like it returns whether nodeProps has an own property matching the name, which means undefined ends up matching since foo exists.

Is that expected?

nodeHasProperty(<div foo={undefined} />, 'foo', undefined) should be true, nodeHasProperty(<div foo={undefined} />, 'foo', null) should be false, nodeHasProperty(<div foo={null} />, 'foo', undefined) should be false.

Right now nodeHasProperty(<div foo={null} />, 'foo', undefined) returns true because it's defaulting to the own-property check. I think this behavior existed before this PR too, is that a bug? I'm not sure if that will break some existing tests.

ping @ljharb ^ I think that's the last point of clarification I need before this is good to go.

In the interest of time, i'm going to move forward w/ this PR. I've verified that this is the current behavior. If we would like to clarify it / change it in an upcoming PR, i'm fine with that. @aweary thanks for being so thorough here.

ljharb · 2017-09-10T17:48:19Z

packages/enzyme-test-suite/test/RSTTraversal-spec.jsx

-
-      expect(() => nodeHasProperty(node, 'title', 'foo')).to.throw();
+      expect(nodeHasProperty(<div foo={Infinity} />, 'foo', Infinity)).to.equal(true);
+      expect(nodeHasProperty(<div foo={Infinity} />, 'foo', +Infinity)).to.equal(true);


It'd also be good to ensure NaN and Infinity don't equate, since both are not finite

ljharb · 2017-09-10T17:48:47Z

packages/enzyme-test-suite/test/ReactWrapper-spec.jsx

@@ -433,22 +435,15 @@ describeWithDOM('mount', () => {
      expect(wrapper.find('.row + .row')).to.have.lengthOf(1);
    });

-    // React 15.2 warns when setting a non valid prop to an DOM element
-    describeIf(REACT013 || REACT014, 'unauthorized dom props', () => {


Yes, totes ok to do in a followup

aweary · 2017-09-13T06:34:06Z

@lelandrichardson a quick heads up, to get the tests passing I had to make a change to the React 16 adapter (a79e150) since the internal instance key was changed in facebook/react@9921e57

lelandrichardson · 2017-09-13T06:36:54Z

@aweary sounds good. thanks.

aweary · 2017-09-13T19:06:37Z

I think I've addressed all the feedback, with the exception of my question here #1086 (comment) about nodeHasProperty behavior. Karma tests are failing on CI, trying to reproduce that locally now.

aweary · 2017-09-13T19:23:55Z

It looks like the Karma tests are failing intermittently. I restarted the build a couple times on Travis and it eventually worked 🤷‍♂️

lelandrichardson · 2017-09-14T06:28:50Z

@aweary do you have a link to a build where it failed? or the error message?

aweary · 2017-09-14T06:50:37Z

@lelandrichardson the last build had the failure with the karma tests and React 16: https://travis-ci.org/airbnb/enzyme/jobs/274910583

It didn't mark the whole build as failed since it's considered an allowed failure, but that's the same error I was seeing before. It looks like some kind of timeout issue.

aweary requested review from ljharb and lelandrichardson August 21, 2017 18:38

aweary force-pushed the css-selector-parser branch from ab6a603 to 38adb22 Compare August 21, 2017 21:02

aweary commented Aug 21, 2017

View reviewed changes

ljharb requested changes Aug 21, 2017

View reviewed changes

aweary commented Aug 22, 2017

View reviewed changes

aweary force-pushed the css-selector-parser branch from 576310d to f976a3e Compare August 23, 2017 18:36

aweary commented Aug 23, 2017

View reviewed changes

aweary changed the title ~~[WIP] Integrate with a CSS parser for selector parsing~~ Integrate with a CSS parser for selector parsing Aug 23, 2017

aweary mentioned this pull request Aug 25, 2017

find with href attribute of full URL is rejected for being complex #283

Closed

aweary commented Aug 25, 2017

View reviewed changes

aweary force-pushed the css-selector-parser branch from 9e1ead2 to 8ddce58 Compare August 30, 2017 16:56

This was referenced Aug 30, 2017

Update RST spec to include a parent and/or sibling pointer(s) #1085

Open

Use postcss-selector-parser for selector handling #458

Closed

ljharb reviewed Sep 1, 2017

View reviewed changes

ljharb added the v3 blocker label Sep 1, 2017

aweary commented Sep 1, 2017

View reviewed changes

aweary force-pushed the css-selector-parser branch from 8ddce58 to 8e3a352 Compare September 1, 2017 22:34

lelandrichardson reviewed Sep 10, 2017

View reviewed changes

lelandrichardson approved these changes Sep 10, 2017

View reviewed changes

ljharb approved these changes Sep 10, 2017

View reviewed changes

Implement new selector parsing module

8177e26

Brandon Dail added 11 commits September 12, 2017 19:29

Replace scalpel with new rst-selector-parser package

8b3e008

Throw for pseudo-class and psuedo-element selectors

1a73724

Add more tests for selectors

36e10d2

Add test for URLS in attribute values

ae733ee

Improve test coverage, use correct APIs

60b6a86

Remove uuid from enzyme package.json

2c3227b

Fix error type in selector assertion

d0d810e

Fix react prop test assertion

459108a

Use if/else for branching in selector predicate functions

188ed1c

Add nodeHasProperty test for NaN and Infinity

17f366d

Add test for adjacent siblings in arrays

aadbc4a

aweary force-pushed the css-selector-parser branch from 088d3c9 to aadbc4a Compare September 13, 2017 03:04

Use _reactInternalFiber for 16 adapater

a79e150

lelandrichardson merged commit 2151b1c into enzymejs:master Sep 16, 2017

lelandrichardson mentioned this pull request Sep 18, 2017

Support nested/multiple find calls #1131

Merged

ljharb added this to Closed/Merged in Make string selectors awesome Sep 26, 2017

samit4me mentioned this pull request Oct 20, 2017

Supported Node Version #1285

Closed

ljharb mentioned this pull request Jan 18, 2019

is ":not" selector not supported? #456

Closed

craigkovatch mentioned this pull request Oct 24, 2019

Error: Failed to parse :not attribute value selector #2268

Open

13 tasks

		@@ -308,14 +308,6 @@ export function coercePropValue(propName, propValue) {

		const trimmedValue = propValue.trim();

		// if propValue includes quotes, it should be

Integrate with a CSS parser for selector parsing #1086

Integrate with a CSS parser for selector parsing #1086

Conversation

aweary commented Aug 21, 2017 • edited by ljharb

Major Changes

Pending Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Aug 21, 2017

aweary commented Aug 22, 2017 • edited

Choose a reason for hiding this comment

aweary commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Aug 29, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lelandrichardson left a comment

Choose a reason for hiding this comment

lelandrichardson commented Sep 10, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Aug 21, 2017 •

edited by ljharb

aweary commented Aug 22, 2017 •

edited

aweary commented Sep 13, 2017 •

edited