New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run stricter linter to cleanup problems and force ES6 usage #566
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to improve, but the rest looks good
|
||
const argv = require('minimist')(process.argv.slice(2)) | ||
|
||
const mochaOpts = assign({ | ||
const mochaOpts = Object.assign({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
const mochaOpts = {
...argv,
ui: 'bdd',
reporter: 'dot',
timeout: 30000
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JPeer264 That's object spread properties; which are only supported in Node 8+, so we can't use those yet. Someday, though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dang! I guess I am using too much babel in my projects. Good to know though ;)
lib/fs/index.js
Outdated
return fs.write(fd, buffer, a, b, c, callback) | ||
exports.write = function (fd, buffer, ...args) { | ||
if (typeof args[args.length - 1] === 'function') { | ||
return fs.write(fd, buffer, ...args) | ||
} | ||
|
||
// Check for old, depricated fs.write(fd, string[, position[, encoding]], callback) | ||
if (typeof buffer === 'string') { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here is actually code duplication now, and this return
is not any different than to the other one.
I think we can leave that if
, because here it does not make the difference if there are two or three arguments. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right; will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I started work to use rest/spread operators, since they're supported in Node 6+. Along the way, I discovered and fixed a few more problems, like regular functions being used instead of arrow functions, missing
'use strict'
statements, usinglet
instead ofconst
, aswitch
whereif
/else
does the job fine, etc.write()
's promise implementation inlib/fs/index.js
required heavy refactoring to avoid usingarguments
, please review this carefully.I also discovered that we're still using our own ponyfill for
Object.assign()
, despite the fact it's now well supported in Node. So I removed that, and refactoredlib/index.js
while I was at it.