Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

requireAlignedMultilineParams: new rule #1901

Closed
wants to merge 2 commits into from
Closed

requireAlignedMultilineParams: new rule #1901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0929797
requireAlignedMultilineParams: enforce indentation of parameters in
JGulbronson Oct 18, 2015
c842dfd
Increased code coverage and documentation
Oct 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/config/configuration.js
View file
Open in desktop
Expand Up @@ -1073,6 +1073,8 @@ Configuration.prototype.registerDefaultRules = function() {
this.registerRule(require('../rules/validate-order-in-object-keys'));

this.registerRule(require('../rules/disallow-tabs'));

this.registerRule(require('../rules/require-aligned-multiline-params'));
};

/**
Expand Down
117 changes: 117 additions & 0 deletions lib/rules/require-aligned-multiline-params.js
View file
Open in desktop
@@ -0,0 +1,117 @@
/**
* Enforces indentation of parameters in multiline functions
*
* Types: `Boolean`, `String`, `Number`
*
* Values:
* - `true` to require parameters are aligned with the body of the function
* - `'firstParam'` to require parameters to be aligned with the first parameter
* - `Number` for the number of columns the parameters should be indented past the function body
*
* #### Example
*
* ```js
* "disallowSpaceBeforeComma": true
* ```
*
* ##### Valid for `true`
*
* ```js
* var test = function(one, two,
* three, four, five,
* six, seven, eight) {
* console.log(a);
* };
* ```
*
* ##### Valid for `2`
*
* ```js
* var test = function(one, two,
* three, four, five,
* six, seven, eight) {
* console.log(a);
* };
* ```
*
* ##### Valid for `'firstParam'`
*
* ```js
* var test = function(one, two,
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add invalid case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

* three, four, five,
* six, seven, eight) {
* console.log(a);
* };
* ```
*
*/

var assert = require('assert');

module.exports = function() {
};

module.exports.prototype = {

configure: function(option) {
if (typeof option === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

We want to switch to https://github.com/mafintosh/is-my-json-valid, to simplify those assertions, we can either leave this as is or you can help us out and simplify live of developers after you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd lean towards leaving it as is, since I don't see the benefit of switching to is-my-json-valid for this particular case. What do you think is the main benefit of switching here?

Copy link
Member

Choose a reason for hiding this comment

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

In your case, close to none, since you already wrote the logic and tests for it, for supporting it, there some benefit and for developers that could use this as an example for the new rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I'd prefer to leave it as is. The tests/logic is already done, and I don't think this is the best example to use it in. Here, we're only validating the type/value of the option parameter. It would feel like overkill to use that library in this scenario, as we're not checking JSON.

Copy link
Member

Choose a reason for hiding this comment

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

Soon we will start to use it even in most simplest cases, no sweat though, as-is is fine, will try it out on the next rule :-)

this._indentationLevel = option;
} else if (typeof option === 'string') {
assert(
option === 'firstParam',
this.getOptionName() + ' option requires string value to be "firstParam"'
);

this._alignWithFirstParam = true;
} else if (option === true) {
this._indentationLevel = 0;
} else {
assert(
false,
this.getOptionName() + ' option requires a valid option'
);
}
},

getOptionName: function() {
return 'requireAlignedMultilineParams';
},

check: function(file, errors) {
var _this = this;
file.iterateNodesByType(['FunctionDeclaration', 'FunctionExpression'], function(node) {
var params = node.params;

// We can pass the check if there's no params
if (params.length === 0) {
return;
}

var currentLine = params[0].loc.start.line;
var referenceColumn;
if (_this._alignWithFirstParam) {
referenceColumn = params[0].loc.start.column;
} else {
referenceColumn = node.body.body[0].loc.start.column + _this._indentationLevel;
}

params.forEach(function(param) {
if (param.loc.start.line !== currentLine) {
if (param.loc.start.column !== referenceColumn) {
errors.assert.indentation({
lineNumber: param.loc.start.line,
actual: param.loc.start.column,
expected: referenceColumn,
indentChar: ' ',
silent: false
});
}

currentLine = param.loc.start.line;
}
});

});
}

};
177 changes: 177 additions & 0 deletions test/specs/rules/require-aligned-multiline-params.js
View file
Open in desktop
@@ -0,0 +1,177 @@
var Checker = require('../../../lib/checker');
var expect = require('chai').expect;

describe('rules/require-aligned-multiline-params', function() {
var checker;
var option;
function rules() { return { 'requireAlignedMultilineParams': option }; }

describe('when we pass true as the config option', function() {
beforeEach(function() {
checker = new Checker();
checker.registerDefaultRules();
option = true;
checker.configure(rules());
});

it('should validate a function with no params properly', function() {
var noParamFunction = 'var noParamFunction = function() { \n' +
' return;\n' +
'};';

expect(checker.checkString(noParamFunction)).to.have.no.errors();
});

it('should validate a function with a single line of params properly', function() {
var singleLineFunction = 'var singleLineFunction = function(a, b, c) { \n' +
' console.log(a + b + c);\n' +
'};';

expect(checker.checkString(singleLineFunction)).to.have.no.errors();
});

it('should validate a function with aligned params properly', function() {
var unalignedFunction = 'var alignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.no.errors();
});

it('should validate a function with one unaligned param properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.one.validation.error.from(
'requireAlignedMultilineParams');
});

it('should validate a function with two unaligned params properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.error.count.equal(2);
});
});

describe('when we pass a number as the config option', function() {
beforeEach(function() {
checker = new Checker();
checker.registerDefaultRules();
option = 2;
checker.configure(rules());
});

it('should validate a function with no params properly', function() {
var noParamFunction = 'var noParamFunction = function() { \n' +
' return;\n' +
'};';

expect(checker.checkString(noParamFunction)).to.have.no.errors();
});

it('should validate a function with a single line of params properly', function() {
var singleLineFunction = 'var singleLineFunction = function(a, b, c) { \n' +
' console.log(a + b + c);\n' +
'};';

expect(checker.checkString(singleLineFunction)).to.have.no.errors();
});

it('should validate a function with aligned params properly', function() {
var unalignedFunction = 'var alignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.no.errors();
});

it('should validate a function with one unaligned param properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.one.validation.error.from(
'requireAlignedMultilineParams');
});

it('should validate a function with two unaligned params properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.error.count.equal(2);
});
});

describe('when we pass "firstParam" as the config option', function() {
beforeEach(function() {
checker = new Checker();
checker.registerDefaultRules();
option = 'firstParam';
checker.configure(rules());
});

it('should validate a function with no params properly', function() {
var noParamFunction = 'var noParamFunction = function() { \n' +
' return;\n' +
'};';

expect(checker.checkString(noParamFunction)).to.have.no.errors();
});

it('should validate a function with a single line of params properly', function() {
var singleLineFunction = 'var singleLineFunction = function(a, b, c) { \n' +
' console.log(a + b + c);\n' +
'};';

expect(checker.checkString(singleLineFunction)).to.have.no.errors();
});

it('should validate a function with aligned params properly', function() {
var unalignedFunction = 'var alignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.no.errors();
});

it('should validate a function with one unaligned param properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.one.validation.error.from(
'requireAlignedMultilineParams');
});

it('should validate a function with two unaligned params properly', function() {
var unalignedFunction = 'var unalignedFunction = function(a,\n' +
' b, c,\n' +
' d, e) {\n' +
' console.log(a + b + c + d + e);\n' +
'};';

expect(checker.checkString(unalignedFunction)).to.have.error.count.equal(2);
});
});
});