Skip to content

tools: lint for function argument alignment #6268

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.

Sign up for GitHub

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

Closed
Trott wants to merge 2 commits into from
Closed

tools: lint for function argument alignment #6268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f20b00
benchmark,lib,test,tools: align function arguments
Trott Apr 19, 2016
74adfbc
tools: lint for function argument alignment
Trott Apr 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ rules:
prefer-const: 2

# Custom rules in tools/eslint-rules
align-function-arguments: 2
align-multiline-assignment: 2
assert-fail-single-argument: 2
new-with-error: [2, "Error", "RangeError", "TypeError", "SyntaxError", "ReferenceError"]
Expand Down
22 changes: 12 additions & 10 deletions benchmark/tls/tls-connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,18 @@ function onConnection(conn) {
}

function makeConnection() {
var conn = tls.connect({ port: common.PORT,
rejectUnauthorized: false }, function() {
clientConn++;
conn.on('error', function(er) {
console.error('client error', er);
throw er;
});
conn.end();
if (running) makeConnection();
});
var conn = tls.connect(
{ port: common.PORT, rejectUnauthorized: false },
function() {
clientConn++;
conn.on('error', function(er) {
console.error('client error', er);
throw er;
});
conn.end();
if (running) makeConnection();
}
);
}

function done() {
Expand Down
19 changes: 9 additions & 10 deletions lib/crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,13 +662,12 @@ function filterDuplicates(names) {
}

// Legacy API
exports.__defineGetter__('createCredentials',
internalUtil.deprecate(function() {
return require('tls').createSecureContext;
}, 'crypto.createCredentials is deprecated. ' +
'Use tls.createSecureContext instead.'));

exports.__defineGetter__('Credentials', internalUtil.deprecate(function() {
return require('tls').SecureContext;
}, 'crypto.Credentials is deprecated. ' +
'Use tls.createSecureContext instead.'));
function defineLegacyGetter(legacyName, tlsName) {
function wrapped() {
return require('tls')[tlsName];
}
const msg = `crypto.${legacyName} is deprecated. Use tls.${tlsName} instead.`;
exports.__defineGetter__(legacyName, internalUtil.deprecate(wrapped, msg));
}
defineLegacyGetter('createCredentials', 'createSecureContext');
defineLegacyGetter('Credentials', 'SecureContext');
20 changes: 13 additions & 7 deletions lib/readline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,17 +1055,23 @@ function codePointAt(str, index) {
}
return code;
}
exports.codePointAt = internalUtil.deprecate(codePointAt,
'readline.codePointAt is deprecated. ' +
'Use String.prototype.codePointAt instead.');
exports.codePointAt = internalUtil.deprecate(
codePointAt,
'readline.codePointAt is deprecated. ' +
'Use String.prototype.codePointAt instead.'
);


exports.getStringWidth = internalUtil.deprecate(getStringWidth,
'getStringWidth is deprecated and will be removed.');
exports.getStringWidth = internalUtil.deprecate(
getStringWidth,
'getStringWidth is deprecated and will be removed.'
);


exports.isFullWidthCodePoint = internalUtil.deprecate(isFullWidthCodePoint,
'isFullWidthCodePoint is deprecated and will be removed.');
exports.isFullWidthCodePoint = internalUtil.deprecate(
isFullWidthCodePoint,
'isFullWidthCodePoint is deprecated and will be removed.'
);


exports.stripVTControlCharacters = internalUtil.deprecate(
Expand Down
4 changes: 2 additions & 2 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,15 +541,15 @@ function formatArray(ctx, value, recurseTimes, visibleKeys, keys) {
for (var i = 0, l = value.length; i < l; ++i) {
if (hasOwnProperty(value, String(i))) {
output.push(formatProperty(ctx, value, recurseTimes, visibleKeys,
String(i), true));
String(i), true));
} else {
output.push('');
}
}
keys.forEach(function(key) {
if (typeof key === 'symbol' || !key.match(/^\d+$/)) {
output.push(formatProperty(ctx, value, recurseTimes, visibleKeys,
key, true));
key, true));
}
});
return output;
Expand Down
4 changes: 2 additions & 2 deletions test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ exports.mustCall = function(fn, expected) {
var etcServicesFileName = path.join('/etc', 'services');
if (exports.isWindows) {
etcServicesFileName = path.join(process.env.SystemRoot, 'System32', 'drivers',
'etc', 'services');
'etc', 'services');
}

/*
Expand Down Expand Up @@ -428,7 +428,7 @@ exports.getServiceName = function getServiceName(port, protocol) {

try {
var servicesContent = fs.readFileSync(etcServicesFileName,
{ encoding: 'utf8'});
{ encoding: 'utf8'});
var regexp = `^(\\w+)\\s+\\s${port}/${protocol}\\s`;
var re = new RegExp(regexp, 'm');

Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dgram-multicast-multi-process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ function launchChildProcess(index) {
worker.pid, count);

assert.strictEqual(count, messages.length,
'A worker received an invalid multicast message');
'A worker received an invalid multicast message');
});

clearTimeout(timer);
Expand Down
36 changes: 20 additions & 16 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ assert.doesNotThrow(makeBlock(a.ok, true),
assert.doesNotThrow(makeBlock(a.ok, 'test'), 'ok(\'test\')');

assert.throws(makeBlock(a.equal, true, false),
a.AssertionError, 'equal(true, false)');
a.AssertionError, 'equal(true, false)');

assert.doesNotThrow(makeBlock(a.equal, null, null),
'equal(null, null)');
Expand Down Expand Up @@ -61,9 +61,10 @@ assert.doesNotThrow(makeBlock(a.notStrictEqual, 2, '2'),

// deepEquals joy!
// 7.2
assert.doesNotThrow(makeBlock(a.deepEqual, new Date(2000, 3, 14),
new Date(2000, 3, 14)),
'deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))');
assert.doesNotThrow(
makeBlock(a.deepEqual, new Date(2000, 3, 14), new Date(2000, 3, 14)),
'deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))'
);

assert.throws(makeBlock(a.deepEqual, new Date(), new Date(2000, 3, 14)),
a.AssertionError,
Expand Down Expand Up @@ -163,10 +164,10 @@ assert.doesNotThrow(makeBlock(a.deepEqual, new Boolean(true), {}),
a.AssertionError);

//deepStrictEqual
assert.doesNotThrow(makeBlock(a.deepStrictEqual, new Date(2000, 3, 14),
new Date(2000, 3, 14)),
'deepStrictEqual(new Date(2000, 3, 14),\
new Date(2000, 3, 14))');
assert.doesNotThrow(
makeBlock(a.deepStrictEqual, new Date(2000, 3, 14), new Date(2000, 3, 14)),
'deepStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14))'
);

assert.throws(makeBlock(a.deepStrictEqual, new Date(), new Date(2000, 3, 14)),
a.AssertionError,
Expand Down Expand Up @@ -331,10 +332,13 @@ assert.throws(function() {assert.ifError(new Error('test error'));});
assert.doesNotThrow(function() {assert.ifError(null);});
assert.doesNotThrow(function() {assert.ifError();});

assert.throws(() => {
assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
}, /Got unwanted exception. user message/,
'a.doesNotThrow ignores user message');
assert.throws(
() => {
Copy link
Member

@benjamingr benjamingr Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was moving the arrow a line down necessary here for the rule?

Copy link
Member Author

@Trott Trott Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't the only option, but yes. Moving it meant that the next line didn't have to be indented so much that it exceeded the 80-character limit on line length.

Copy link
Member

@jasnell jasnell Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally not a fan of moving the first argument down if it can be avoided but can live with this.

Copy link
Member Author

@Trott Trott Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell There are certainly other options, like assigning the function to a variable so you can just do assert.throws(foo,...).

Copy link
Member Author

@Trott Trott Apr 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the previous indentation is, in my opinion at least, unfriendly to the reader:

assert.throws(() => {
  assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
}, /Got unwanted exception. user message/,
   'a.doesNotThrow ignores user message');

I think moving the arrow function down one line is worth getting code that is easier to understand at a glance:

assert.throws(
  () => {
    assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
  },
  /Got unwanted exception. user message/,
  'a.doesNotThrow ignores user message'
);

assert.doesNotThrow(makeBlock(thrower, Error), 'user message');
},
/Got unwanted exception. user message/,
'a.doesNotThrow ignores user message'
);

// make sure that validating using constructor really works
threw = false;
Expand Down Expand Up @@ -376,7 +380,7 @@ try {
} catch (e) {
threw = true;
assert(e instanceof AnotherErrorType,
`expected AnotherErrorType, received ${e}`);
`expected AnotherErrorType, received ${e}`);
}

assert.ok(threw);
Expand Down Expand Up @@ -410,7 +414,7 @@ function testAssertionMessage(actual, expected) {
assert.equal(actual, '');
} catch (e) {
assert.equal(e.toString(),
['AssertionError:', expected, '==', '\'\''].join(' '));
['AssertionError:', expected, '==', '\'\''].join(' '));
assert.ok(e.generatedMessage, 'Message not marked as generated');
}
}
Expand All @@ -436,7 +440,7 @@ testAssertionMessage({}, '{}');
testAssertionMessage(circular, '{ y: 1, x: [Circular] }');
testAssertionMessage({a: undefined, b: null}, '{ a: undefined, b: null }');
testAssertionMessage({a: NaN, b: Infinity, c: -Infinity},
'{ a: NaN, b: Infinity, c: -Infinity }');
'{ a: NaN, b: Infinity, c: -Infinity }');

// #2893
try {
Expand All @@ -462,7 +466,7 @@ try {
} catch (e) {
assert.equal(e.toString().split('\n')[0], 'AssertionError: oh no');
assert.equal(e.generatedMessage, false,
'Message incorrectly marked as generated');
'Message incorrectly marked as generated');
}

// Verify that throws() and doesNotThrow() throw on non-function block
Expand Down
48 changes: 30 additions & 18 deletions test/parallel/test-buffer-alloc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,8 @@ for (let i = 0; i < Buffer.byteLength(utf8String); i++) {
{
const f = Buffer.from('привет', encoding);
console.error('f.length: %d (should be 12)', f.length);
assert.deepStrictEqual(f,
Buffer.from([63, 4, 64, 4, 56, 4, 50, 4, 53, 4, 66, 4]));
const expected = Buffer.from([63, 4, 64, 4, 56, 4, 50, 4, 53, 4, 66, 4]);
assert.deepStrictEqual(f, expected);
assert.equal(f.toString(encoding), 'привет');
}

Expand Down Expand Up @@ -645,18 +645,30 @@ assert.equal(Buffer.from('KioqKioqKioqKioqKioqKioqKio', 'base64').toString(),
'********************');

// handle padding graciously, multiple-of-4 or not
assert.equal(Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw==',
'base64').length, 32);
assert.equal(Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw=',
'base64').length, 32);
assert.equal(Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw',
'base64').length, 32);
assert.equal(Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg==',
'base64').length, 31);
assert.equal(Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg=',
'base64').length, 31);
assert.equal(Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg',
'base64').length, 31);
assert.equal(
Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw==', 'base64').length,
32
);
assert.equal(
Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw=', 'base64').length,
32
);
assert.equal(
Buffer.from('72INjkR5fchcxk9+VgdGPFJDxUBFR5/rMFsghgxADiw', 'base64').length,
32
);
assert.equal(
Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg==', 'base64').length,
31
);
assert.equal(
Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg=', 'base64').length,
31
);
assert.equal(
Buffer.from('w69jACy6BgZmaFvv96HG6MYksWytuZu3T1FvGnulPg', 'base64').length,
31
);

// This string encodes single '.' character in UTF-16
var dot = Buffer.from('//4uAA==', 'base64');
Expand Down Expand Up @@ -1131,16 +1143,16 @@ assert.throws(function() {
var buf = Buffer.from([0xFF, 0xFF, 0xFF, 0xFF]);

assert.equal(buf['readUInt' + bits + 'BE'](0),
(0xFFFFFFFF >>> (32 - bits)));
0xFFFFFFFF >>> (32 - bits));

assert.equal(buf['readUInt' + bits + 'LE'](0),
(0xFFFFFFFF >>> (32 - bits)));
0xFFFFFFFF >>> (32 - bits));

assert.equal(buf['readInt' + bits + 'BE'](0),
(0xFFFFFFFF >> (32 - bits)));
0xFFFFFFFF >> (32 - bits));

assert.equal(buf['readInt' + bits + 'LE'](0),
(0xFFFFFFFF >> (32 - bits)));
0xFFFFFFFF >> (32 - bits));
});

// test for common read(U)IntLE/BE
Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-buffer-bytelength.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,10 @@ assert.equal(Buffer.byteLength('ßœ∑≈', 'unkn0wn enc0ding'), 10);
assert.equal(Buffer.byteLength('aGVsbG8gd29ybGQ=', 'base64'), 11);
assert.equal(Buffer.byteLength('bm9kZS5qcyByb2NrcyE=', 'base64'), 14);
assert.equal(Buffer.byteLength('aGkk', 'base64'), 3);
assert.equal(Buffer.byteLength('bHNrZGZsa3NqZmtsc2xrZmFqc2RsZmtqcw==',
'base64'), 25);
assert.equal(
Buffer.byteLength('bHNrZGZsa3NqZmtsc2xrZmFqc2RsZmtqcw==', 'base64'),
25
);
// special padding
assert.equal(Buffer.byteLength('aaa=', 'base64'), 2);
assert.equal(Buffer.byteLength('aaaa==', 'base64'), 3);
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-buffer-includes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ assert(!mixedByteStringUcs2.includes('\u0396', 0, 'ucs2'));
assert(
6, mixedByteStringUcs2.includes(Buffer.from('bc', 'ucs2'), 0, 'ucs2'));
assert(
10, mixedByteStringUcs2.includes(Buffer.from('\u03a3', 'ucs2'),
0, 'ucs2'));
10,
mixedByteStringUcs2.includes(Buffer.from('\u03a3', 'ucs2'), 0, 'ucs2'));
assert(
-1, mixedByteStringUcs2.includes(Buffer.from('\u0396', 'ucs2'),
0, 'ucs2'));
-1,
mixedByteStringUcs2.includes(Buffer.from('\u0396', 'ucs2'), 0, 'ucs2'));

twoByteString = Buffer.from('\u039a\u0391\u03a3\u03a3\u0395', 'ucs2');

Expand Down
33 changes: 22 additions & 11 deletions test/parallel/test-buffer-indexof.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,43 +81,54 @@ assert.equal(Buffer.from('ff').indexOf(Buffer.from('f'), 1, 'ucs2'), -1);
// test hex encoding
assert.equal(
Buffer.from(b.toString('hex'), 'hex')
.indexOf('64', 0, 'hex'), 3);
.indexOf('64', 0, 'hex'),
Copy link
Member

@targos targos Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't multiline method calls be indented with 2 spaces ?

Copy link
Member Author

@Trott Trott Apr 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@targos asked:

shouldn't multiline method calls be indented with 2 spaces ?

(Deleted my previous inaccurate response.)

This is a bug in ESLint. As @nzakas comments in that issue:

Yeah, I'd imagine this will be open for quite some time. It's pretty difficult to do correctly, but patches always welcome.

(That comment is from August.)

Copy link
Member Author

@Trott Trott Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@targos I've made an effort to enforce indentation in cases like this and submitted the PR upstream. eslint/eslint#5940

Copy link
Member Author

@Trott Trott Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, in case you're curious, that rule finds 110 instances of mis-aligned chained properties in 18 files (3 in lib and 15 in test).

Copy link
Member Author

@Trott Trott Apr 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I don't think it's really going to fly in this project. There's going to definitely be people that will prefer this:

var a = foo.bar()
           .baz()
           .bip()
           .bap();

...instead of:

var a = foo.bar()
  .baz()
  .bip()
  .bap();

It may be possible to write a rule smart enough to distinguish between that and other cases where it may make sense to indent-by-two rather than align, but I suspect there will be many edge cases. Indentation of then() in promise chains would seem to be a likely one too.

Copy link
Member Author

@Trott Trott Apr 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution might be to write a rule enforcing alignment (as in the first example in the previous comment).

3);
assert.equal(
Buffer.from(b.toString('hex'), 'hex')
.indexOf(Buffer.from('64', 'hex'), 0, 'hex'), 3);
.indexOf(Buffer.from('64', 'hex'), 0, 'hex'),
3);

// test base64 encoding
assert.equal(
Buffer.from(b.toString('base64'), 'base64')
.indexOf('ZA==', 0, 'base64'), 3);
.indexOf('ZA==', 0, 'base64'),
3);
assert.equal(
Buffer.from(b.toString('base64'), 'base64')
.indexOf(Buffer.from('ZA==', 'base64'), 0, 'base64'), 3);
.indexOf(Buffer.from('ZA==', 'base64'), 0, 'base64'),
3);

// test ascii encoding
assert.equal(
Buffer.from(b.toString('ascii'), 'ascii')
.indexOf('d', 0, 'ascii'), 3);
.indexOf('d', 0, 'ascii'),
3);
assert.equal(
Buffer.from(b.toString('ascii'), 'ascii')
.indexOf(Buffer.from('d', 'ascii'), 0, 'ascii'), 3);
.indexOf(Buffer.from('d', 'ascii'), 0, 'ascii'),
3);

// test binary encoding
assert.equal(
Buffer.from(b.toString('binary'), 'binary')
.indexOf('d', 0, 'binary'), 3);
.indexOf('d', 0, 'binary'),
3);
assert.equal(
Buffer.from(b.toString('binary'), 'binary')
.indexOf(Buffer.from('d', 'binary'), 0, 'binary'), 3);
.indexOf(Buffer.from('d', 'binary'), 0, 'binary'),
3);
assert.equal(
Buffer.from('aa\u00e8aa', 'binary')
.indexOf('\u00e8', 'binary'), 2);
.indexOf('\u00e8', 'binary'),
2);
assert.equal(
Buffer.from('\u00e8', 'binary')
.indexOf('\u00e8', 'binary'), 0);
.indexOf('\u00e8', 'binary'),
0);
assert.equal(
Buffer.from('\u00e8', 'binary')
.indexOf(Buffer.from('\u00e8', 'binary'), 'binary'), 0);
.indexOf(Buffer.from('\u00e8', 'binary'), 'binary'),
0);


// test optional offset with passed encoding
Expand Down
Loading