Add JS Standard linting an run in Travis #26

geymed · 2017-10-01T21:12:13Z

Add JS Standard
Add script to run linting
Fix linting errors
Run lint and tests in Travis CI

Fixes #15

joeljuca · 2017-10-03T03:47:20Z

Thanks for starting this work. Here are some things I would like you to do:

Remove changes not related to the issue itself. I know that it's a good thing to have .vscode and npm-debug.log in .gitignore, but it's also very important to be able to git revert a contribution without losing any other functionality;
(It's also a best practice for Open Source projects. You'll have your contributions accepted more easily if they are focused on solving one issue at a time)
You don't need to explicitly state that npm install is how Node.js installs stuff since it's the default behavior;
please keep the spacing on module loading sections. It will probably be the only exception I'll take on standard. Keeping the assignment operators aligned makes the code way cleaner than otherwise;
You can hide a certain warning using ESLint disabling features to hide the no-multi-spaces one (example below);

/* eslint-disable no-multi-spaces */
const program = require('commander')
const package = require('../package.json')
const pkg     = require('../package.json')
const config  = require('../lib/config')
/* eslint-enable no-multi-spaces */

I'll also add inline comments as a formal review.

joeljuca · 2017-10-03T04:31:15Z

FYI, I just added the standard output of all files in the description of #15.

geymed · 2017-10-03T18:37:16Z

Hey @joelwallis ,
Thanks for your comments.
I believe I've addressed all of them, LMK if there's still anything missing / need to be fixed.

joeljuca · 2017-10-03T19:55:23Z

Hey @geymed,

I just cloned your fork and ran standard on it. It still throws lots of errors (see below). Most of them are the same as others already fixed in other files - like variables named package and padded blocks. Surely a low-hanging fruit.

$ standard
standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /tmp/geymed-dsafio/bin/dsafio-config.js:4:7: Parsing error: The keyword 'package' is reserved
  /tmp/geymed-dsafio/bin/dsafio.js:4:7: Parsing error: The keyword 'package' is reserved
  /tmp/geymed-dsafio/lib/fs-as-promise.js:1:12: Multiple spaces found before '='.
  /tmp/geymed-dsafio/lib/registry.js:1:15: Multiple spaces found before '='.
  /tmp/geymed-dsafio/lib/registry.js:2:7: 'path' is assigned a value but never used.
  /tmp/geymed-dsafio/lib/registry.js:2:15: Multiple spaces found before '='.
  /tmp/geymed-dsafio/lib/registry.js:3:7: 'util' is assigned a value but never used.
  /tmp/geymed-dsafio/lib/registry.js:3:15: Multiple spaces found before '='.
  /tmp/geymed-dsafio/lib/registry.js:5:15: Multiple spaces found before '='.
  /tmp/geymed-dsafio/lib/registry.js:63:18: 'new Buffer()' was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' (use 'https://www.npmjs.com/package/safe-buffer' for '<4.5.0') instead.
  /tmp/geymed-dsafio/test/lib/config.test.js:1:14: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/config.test.js:2:14: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/config.test.js:4:14: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/config.test.js:9:1: 'describe' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:9:36: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/config.test.js:11:3: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:13:3: 'describe' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:13:33: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/config.test.js:15:5: 'beforeEach' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:19:5: 'afterEach' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:21:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:23:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:30:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:36:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:52:3: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/config.test.js:54:3: 'describe' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:54:33: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/config.test.js:56:5: 'beforeEach' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:60:5: 'afterEach' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:65:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:67:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:71:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:75:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:81:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/config.test.js:84:11: Expected an assignment or function call and instead saw an expression.
  /tmp/geymed-dsafio/test/lib/config.test.js:85:11: Expected an assignment or function call and instead saw an expression.
  /tmp/geymed-dsafio/test/lib/config.test.js:89:3: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/config.test.js:91:1: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/registry.test.js:1:16: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/registry.test.js:2:16: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/registry.test.js:3:16: Multiple spaces found before '='.
  /tmp/geymed-dsafio/test/lib/registry.test.js:8:1: 'describe' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:8:38: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/registry.test.js:10:3: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:12:3: 'describe' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:12:33: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/registry.test.js:14:5: 'beforeEach' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:21:5: 'afterEach' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:26:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:28:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:35:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:41:5: 'it' is not defined.
  /tmp/geymed-dsafio/test/lib/registry.test.js:55:3: Block must not be padded by blank lines.
  /tmp/geymed-dsafio/test/lib/registry.test.js:57:1: Block must not be padded by blank lines.

geymed · 2017-10-03T20:11:29Z

@joelwallis that's weird, did you checkout the feature branch (feature/jsstandard)?
This commit: eb03a59 supposed to fix these errors.

joeljuca · 2017-10-03T23:46:14Z

@geymed My bad! I ran standard on master branch 😂

joeljuca · 2017-10-03T03:48:07Z

.gitignore

 registry.json
+npm-debug.log


Remove these entries. We can add them later.

joeljuca · 2017-10-03T03:49:08Z

.travis.yml

@@ -4,3 +4,9 @@ node_js:
 cache:
  directories:
    - "node_modules"
+install:
+- "npm install" 


npm install is the default command to install dependencies on Travis. No need to have it on .travis.yml

joeljuca · 2017-10-03T03:50:53Z

bin/dsafio-config.js

@@ -16,7 +16,6 @@ if (!['get', 'set'].includes(operation)) {
 }

 switch (operation) {
-


Is standard complaining about padded blocks? If that's so, you should fix all padded blocks (CLI, libs and tests).

@geymed 👆

joeljuca · 2017-10-03T03:52:25Z

lib/registry.js

@@ -60,7 +58,7 @@ function loadFromApi () {
  })
    .then(JSON.parse)
    .then(payload => payload.content)
-    .then(raw => new Buffer(raw, 'base64').toString())
+    .then(raw => Buffer.from(raw, 'base64').toString())


No need of it for this PR. Again: focus on the problem you're solving now.

@geymed 👆

Oh, just saw the standard error. new Buffer() was deprecated. Nevermind.

./lib/registry.js:63:18: 'new Buffer()' was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' (use 'https://www.npmjs.com/package/safe-buffer' for '<4.5.0') instead. (node/no-deprecated-api)

joeljuca · 2017-10-03T03:53:44Z

package.json

@@ -7,7 +7,8 @@
  },
  "scripts": {
    "test": "mocha -b --recursive",
-    "test:watch": "mocha -bw --recursive"
+    "test:watch": "mocha -bw --recursive",
+    "lint": "./node_modules/.bin/standard"


You don't need to use the full path of standard, the node_modules/.bin folder is included in the module path when you're using npm scripts. See: I'm using just mocha on test, not ./node_modules/.bin/mocha. 🙂 It's safe on npm scripts.

Also, remember to include *all CLI, libs and tests .js files:

"lint": "standard ./bin/*.js ./lib/*.js ./test/*/*.js"

@geymed 👆

@joelwallis when I just run standard it scans all files recursively. I can specify paths if there some we want to skip.

Hey @geymed, you can ignore this comment. I saw that standard searches *.js files recursively while ignoring paths like node_modules and ones declared in .gitignore. 🙂

joeljuca · 2017-10-03T04:08:59Z

test/lib/config.test.js


 chai.use(require('chai-as-promised'))
 const expect = chai.expect

+/* eslint-env mocha */


Would you mind moving it to the top of the file (first line)?

joeljuca · 2017-10-03T04:09:36Z

test/lib/registry.test.js

 const registry = require('../../lib/registry')

 const expect = chai.expect

+/* eslint-env mocha */


This one needs to move to the top too.

joeljuca · 2017-10-03T04:10:12Z

test/lib/config.test.js

@@ -78,14 +75,13 @@ describe('lib/config', function () {
      return promise
    })

+    /* eslint no-unused-expressions:0 */


Why did you use this one here?

@geymed 👆

joeljuca · 2017-10-04T21:23:19Z

@geymed I just saw that I didn't submit my review. That's why it wasn't showing for you. My bad!

joeljuca · 2017-10-04T21:40:19Z

@geymed I changed the code directly to streamline this PR on my fork, branch wip-pr-26. Could you please cherry-pick these changes in your branch? This way we can get them inserted when it gets merged to master.

Here are the commands that you'll need to run:

git remote add joelwallis git@github.com:joelwallis/dsafio.git
git pull joelwallis wip-pr-26
git checkout feature/jsstandard
git cherry-pick 1554124

I made the changes on my fork - https://github.com/joelwallis/dsafio/tree/wip-pr-26

joeljuca · 2017-10-05T16:07:08Z

@geymed I made just a final change request for this PR. It's all good.

geymed · 2017-10-05T19:56:49Z

@joelwallis just cherry-picked your commit, LMK if it's OK.
Thanks!

joeljuca · 2017-10-05T20:01:17Z

Great work @geymed!

Feel free to proceed with the dsafio/challenges#3 one 😊

geymed mentioned this pull request Oct 1, 2017

JavaScript Standard Style on Travis CI #15

Closed

7 tasks

geymed added 3 commits October 3, 2017 21:26

Add Standard JS linting

e7b6095

Run lint and tests in Travis CI

b13cf7b

Fix lint errors

eb03a59

geymed force-pushed the feature/jsstandard branch from f644e9e to eb03a59 Compare October 3, 2017 18:34

joeljuca mentioned this pull request Oct 3, 2017

JavaScript Standard Style on CI dsafio/challenges#2

Closed

joeljuca previously requested changes Oct 4, 2017

View reviewed changes

Minor tweaks for PR on JavaScript Standard Style

1554124

joeljuca merged commit 837baa4 into dsafio:master Oct 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JS Standard linting an run in Travis #26

Add JS Standard linting an run in Travis #26

geymed commented Oct 1, 2017 •

edited

Loading

joeljuca commented Oct 3, 2017

joeljuca commented Oct 3, 2017

geymed commented Oct 3, 2017 •

edited

Loading

joeljuca commented Oct 3, 2017

geymed commented Oct 3, 2017 •

edited

Loading

joeljuca commented Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 4, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

geymed Oct 5, 2017

joeljuca Oct 5, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca Oct 3, 2017

joeljuca commented Oct 4, 2017

joeljuca commented Oct 4, 2017

joeljuca commented Oct 5, 2017

geymed commented Oct 5, 2017

joeljuca commented Oct 5, 2017

		@@ -16,7 +16,6 @@ if (!['get', 'set'].includes(operation)) {
		}

		switch (operation) {

		registry.json
		npm-debug.log

Add JS Standard linting an run in Travis #26

Add JS Standard linting an run in Travis #26

Conversation

geymed commented Oct 1, 2017 • edited Loading

joeljuca commented Oct 3, 2017

joeljuca commented Oct 3, 2017

geymed commented Oct 3, 2017 • edited Loading

joeljuca commented Oct 3, 2017

geymed commented Oct 3, 2017 • edited Loading

joeljuca commented Oct 3, 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

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

joeljuca commented Oct 4, 2017

joeljuca commented Oct 4, 2017

joeljuca commented Oct 5, 2017

geymed commented Oct 5, 2017

joeljuca commented Oct 5, 2017

geymed commented Oct 1, 2017 •

edited

Loading

geymed commented Oct 3, 2017 •

edited

Loading

geymed commented Oct 3, 2017 •

edited

Loading