Add Makefile.js and eslint #15
Conversation
soda0289
force-pushed
the
makefile.js
branch
3 times, most recently
from
6976bf2 to
dceb154
Compare
|
Great stuff, thanks! I'll let you know as soon as #14 is merged |
soda0289
force-pushed
the
makefile.js
branch
from
dceb154 to
212b206
Compare
|
@soda0289 The build failed: |
|
Do we want code coverage? I will just remove it for now. |
|
Ideally, yes, as that would match up with our other projects. Is it not just a matter of adding istanbul as a devDependency? |
|
Hmm I'm not sure how travis works. I can add it as dependency and call by the node_modues path. Let me take a look at how eslint does it |
|
I got code coverage working just going to fix all these linting errors. Give me an hour. |
soda0289
force-pushed
the
makefile.js
branch
2 times, most recently
from
86bcb70 to
ec7326d
Compare
|
It's going to take a little longer to finish fixing all the linting rules. I have pushed a WIP commit but will finish it later this evening. |
soda0289
force-pushed
the
makefile.js
branch
3 times, most recently
from
7c68b7b to
a000184
Compare
|
@JamesHenry Should be good to go. I disabled some linting rules in the tests, but mostly had to add JSDoc comments and make minor tweaks to get everything passing. |
soda0289
force-pushed
the
makefile.js
branch
from
a000184 to
06234ab
Compare
There was a problem hiding this comment.
Thanks so much for this @soda0289, this can't have been a fun PR to put together 😄
I just noted the one code change that I believe was made inadvertently
src/scope.js
Outdated
| @@ -400,19 +422,19 @@ class Scope { | |||
| } | |||
|
|
|||
| __isClosed() { | |||
| return this.__left === null; | |||
| return this.__left === null || this.__left === undefined; | |||
There was a problem hiding this comment.
This is a code change, I have no idea if it is significant, but thought I should point it out as it is not necessary for the PR
There was a problem hiding this comment.
They had some parts of the code that had != null and == null and I changed those to triple equals with undefined. I can change this one back.
There was a problem hiding this comment.
Yes, I saw, and so you did not change that meaning of the code in those cases. Here you did, that's why I am pointing it out
There was a problem hiding this comment.
As I say, I have no idea what significance that has, but it's probably worth pointing out
There was a problem hiding this comment.
I changed it back. Although the tests pass I dont want to risk introducing new bugs.
There was a problem hiding this comment.
Exactly 👍 Would be an awful one to track down
soda0289
force-pushed
the
makefile.js
branch
from
06234ab to
3b65aa7
Compare
src/pattern-visitor.js
Outdated
| @@ -56,7 +61,7 @@ class PatternVisitor extends esrecurse.Visitor { | |||
| const lastRestElement = getLast(this.restElements); | |||
| this.callback(pattern, { | |||
| topLevel: pattern === this.rootPattern, | |||
| rest: lastRestElement != null && lastRestElement.argument === pattern, | |||
| rest: lastRestElement !== null && lastRestElement.argument === pattern, | |||
There was a problem hiding this comment.
Ah here's another code change
There was a problem hiding this comment.
good catch will fix. Thanks
src/referencer.js
Outdated
| function traverseIdentifierInPattern(options, rootPattern, referencer, callback) { | ||
| // Call the callback at left hand identifier nodes, and Collect right hand nodes. | ||
| var visitor = new PatternVisitor(options, rootPattern, callback); | ||
| visitor.visit(rootPattern); | ||
|
|
||
| // Process the right hand nodes recursively. | ||
| if (referencer != null) { | ||
| if (referencer !== null) { |
soda0289
force-pushed
the
makefile.js
branch
from
3b65aa7 to
49cfd08
Compare
|
I pushed the changes. Think that's all of them. Thanks for the review |
|
LGTM, only thing I'd say is to add an .eslintrc file to the test directory that extends the main .eslintrc file and allows describe and it as globals. |
|
@mikesherov Great Idea. I can set environment to mocha in the test directory and that should handle all the globals. Thanks |
This adds a Makefile that will run the tests, lint the code, and check licenses
soda0289
force-pushed
the
makefile.js
branch
from
49cfd08 to
7eaf45e
Compare
|
Is this finalised now @soda0289? |
|
Yup should be good. |
We currently have the lint target disabled until the babel is removed. The test run but depend on babel-core/register. When #14 is merged I will turn on the linter.