Added autofix functions for blank-lines rule + test cases #161
Conversation
lib/rules/blank-lines.js
Outdated
| // TODO: generalize this method for N consec. blank lines in future | ||
| function has2ConsecutiveBlankLines(string) { | ||
| return /\n[ \t]*\n[ \t]*\n/.test(string); | ||
| let eol = require("os").EOL; |
There was a problem hiding this comment.
use const for all things require()d and use it in place of let wherever you can.
There was a problem hiding this comment.
Sure, I'll apply all the ES6 refactoring changes you requested to the rest of the rule and tests as needed, and not just my code. Might as well kill 2 birds with one stone.
lib/rules/blank-lines.js
Outdated
| // - Insert the desired number of line break characters right before the comment | ||
|
|
||
| let line = sourceCode.getLine(topLevelNode) - 1, // The line number | ||
| maxLines = spacing.split("\n").length, |
There was a problem hiding this comment.
Are you sure we don't need to use EOL instead of just \n over here? And in all the places where you're using \n only?
There was a problem hiding this comment.
The code would be equivalent either way since \r\n includes \n. Although you could argue that splitting by \n alone causes inconsistent behavior across OSes since on windows there would be an extra \r in the substrings resulting from the split. In the context of counting the number of lines it's not really a big deal if the strings have that extra \r. If you'd prefer to use EOL for the sake of consistency however that's a good reason and I could definitely change it up.
There was a problem hiding this comment.
ok that makes sense. But let's use EOL
- for consistency
- so there's no scope of confusion about what kind of linebreak to use where for those who read this rule's code in future
There was a problem hiding this comment.
Sounds good. My latest commit should cover it.
lib/rules/blank-lines.js
Outdated
| let line = sourceCode.getLine(topLevelNode) - 1, // The line number | ||
| maxLines = spacing.split("\n").length, | ||
| bottomLine = sourceCode.getLine(topLevelNode), | ||
| multipleLines = spacing.indexOf("\n") > -1, // Whether there are multiple lines |
There was a problem hiding this comment.
use includes() method in place of indexOf(...) > -1 at all places where you need to check whether an item exists inside an array/string. The indexOf() is a bad habit I introduced long back when I wasn't aware of includes().
eg
// returns boolean
myArray.includes(90)
myString.includes("\n")
| @@ -330,3 +330,150 @@ describe("[RULE] blank-lines: Rejections", function() { | |||
| }); | |||
|
|
|||
| }); | |||
|
|
|||
|
|
|||
| describe("[RULE] blank-lines: Fixes", function() { | |||
There was a problem hiding this comment.
use () => { in place of function() {. In general, use all the ES6 features you can except for import statements. Solium was originally written in es5 so you will see es5-style code at a lot of places but we're now encouraging es6!
| done(); | ||
| }); | ||
|
|
||
| it("Should correct multiline functions not followed by a blank line", function(done) { |
|
@GabrielAlacchi Just minor changes needed, everything looks great! |
GabrielAlacchi
force-pushed
the
blank-lines
branch
from
3238986 to
d367da1
Compare
GabrielAlacchi
force-pushed
the
blank-lines
branch
from
cebc57c to
d5d7dae
Compare
|
awesome! |
This PR is in regards to issue #94. I've added autofix functions for the rule, as well as did some work to ensure compatibility with windows style line endings
\r\nvs\nby using theEOLproperty of node's OS package, as well as by modifying the regular expressions to use\sor\r?\nin certain instances. I've also applied the generalization for thehas2ConsecutiveBlankLines(string)to deal with N consecutive blank lines, thus changing the signature tohasNConsecutiveBlankLines(string, n).There are two autofix functions that have been added.
First is the autofix function for the
inspectChildlinting inspection, which checks if children of top level nodes are succeeded by at least 1 blank line. It simply fixes the problem by inserting a blank line.The second is the autofix function for the
inspectProgramlinting inspection, which checks if top level nodes have 2 spaces preceeding them and any accompanying comments. The fixing function uses the following strategy.The latter fix is elaborate, but it manages to fix all of the test code snippets in the acceptances test case of the blank-lines unit test file. I've copied most of those code snippets as well as writing 1-2 more for the unit tests that have been added for these functions.