Repository navigation
tools: lint for function argument alignment #6390
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev
Previous commit
tools: lint for function argument alignment
In function calls that span multiple lines, apply a custom lint rule to
enforce argument alignment.
With this rule, the following code will be flagged as an error by the
linter because the arguments on the second line start in a different
column than on the first line:
myFunction(a, b,
c, d);
The following code will not be flagged as an error by the linter:
myFunction(a, b,
c, d);- Loading branch information
commit 4b0c0d865c4750168975544103ce4e06e947d35f
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /** | ||
| * @fileoverview Align arguments in multiline function calls | ||
| * @author Rich Trott | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| // Rule Definition | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| function checkArgumentAlignment(context, node) { | ||
|
|
||
| function isNodeFirstInLine(node, byEndLocation) { | ||
| const firstToken = byEndLocation === true ? context.getLastToken(node, 1) : | ||
| context.getTokenBefore(node); | ||
| const startLine = byEndLocation === true ? node.loc.end.line : | ||
| node.loc.start.line; | ||
| const endLine = firstToken ? firstToken.loc.end.line : -1; | ||
|
|
||
| return startLine !== endLine; | ||
| } | ||
|
|
||
| if (node.arguments.length === 0) | ||
| return; | ||
|
|
||
| var msg = ''; | ||
| const first = node.arguments[0]; | ||
| var currentLine = first.loc.start.line; | ||
| const firstColumn = first.loc.start.column; | ||
|
|
||
| const ignoreTypes = [ | ||
| 'ArrowFunctionExpression', | ||
| 'CallExpression', | ||
| 'FunctionExpression', | ||
| 'ObjectExpression', | ||
| 'TemplateLiteral' | ||
| ]; | ||
|
|
||
| const args = node.arguments; | ||
|
|
||
| // For now, don't bother trying to validate potentially complicating things | ||
| // like closures. Different people will have very different ideas and it's | ||
| // probably best to implement configuration options. | ||
| if (args.some((node) => { return ignoreTypes.indexOf(node.type) !== -1; })) { | ||
| return; | ||
| } | ||
|
|
||
| if (!isNodeFirstInLine(node)) { | ||
| return; | ||
| } | ||
|
|
||
| args.slice(1).forEach((argument) => { | ||
| if (argument.loc.start.line === currentLine + 1) { | ||
| if (argument.loc.start.column !== firstColumn) { | ||
| msg = 'Function called with argument in column ' + | ||
| `${argument.loc.start.column}, expected in ${firstColumn}`; | ||
| } | ||
| } | ||
| currentLine = argument.loc.start.line; | ||
| }); | ||
|
|
||
| if (msg) | ||
| context.report(node, msg); | ||
| } | ||
|
|
||
| module.exports = function(context) { | ||
| return { | ||
| 'CallExpression': (node) => checkArgumentAlignment(context, node) | ||
| }; | ||
| }; | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
if (args.some((node) => ignoreTypes.includes(node.type))) {?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.
At least for the moment, that will break on CI. For faster results, the VM that runs the linter uses the system Node.js (currently 5.x) and doesn't compile from source. 5.x does not have
Array.prototype.includes().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.
Although I could get rid of the braces and
returnkeyword... is "no braces for arrow functions where they can be omitted" a style we want to standardize on? ESLint has a built-in rule for that.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.
I prefer the short form because why many words when can terse, right? @jbergstroem Are there any downsides to upgrading the node that runs the linter to v6?