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

New Rule: requirePaddingNewlinesBeforeKeywords #586

Closed
wants to merge 6 commits into from
Closed

New Rule: requirePaddingNewlinesBeforeKeywords #586

wants to merge 6 commits into from

Conversation

avishnyak
Copy link

Requires an empty line above the specified keywords unless the keyword is the first expression in a block.

Configured like so:

"requirePaddingNewlinesBeforeKeywords": ["if", "for", "return", "switch", "case", "break", "throw"]

The intent of the rule is to introduce newlines (whitespace) between expressions. Many libraries and individuals already follow this type of convention intuitively.

The rule simply transforms code like this:

String.prototype.hashCode = function () {
    var hash = 0, i, char;
    if (this.length == 0) { return hash };
    for (i = 0, l = this.length; i < l; i++) {
        char = this.charCodeAt(i);
        hash = ((hash << 5) - hash) + char;
        hash |= 0; // Convert to 32bit integer
    }
    return hash;
};

into code like this:

String.prototype.hashCode = function () {
    var hash = 0, i, char;

    if (this.length == 0) {
        return hash
    };

    for (i = 0, l = this.length; i < l; i++) {
        char = this.charCodeAt(i);
        hash = ((hash << 5) - hash) + char;
        hash |= 0; // Convert to 32bit integer
    }

    return hash;
};

Note the empty lines above the if, for and return.

@avishnyak
Copy link
Author

One interesting proposal from the conversation on Reddit:

Add a configuration for minimum block size before the rule kicks in. This would prevent the rule from suggesting extra padding lines in very short functions/code blocks.

Example:

Widget.prototype.exec = function(n) {
    var method = n > 0 ? 'get' : 'set';
    return this[method](n);
};

I'm open to thoughts/suggestions on this.

@@ -80,6 +80,8 @@ StringChecker.prototype = {
this.registerRule(new (require('./rules/require-padding-newlines-in-blocks'))());
this.registerRule(new (require('./rules/require-newline-before-block-statements'))());
this.registerRule(new (require('./rules/disallow-newline-before-block-statements'))());

this.registerRule(new (require('./rules/require-padding-newlines-before-keywords'))());
Copy link
Member

Choose a reason for hiding this comment

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

Please correct the indentation

@markelog
Copy link
Member

Would you able provide the opposite rule? Could you add more tests? Like one in the doc, it would be preferable to have test for every keywords too.

@avishnyak
Copy link
Author

Hi Markelog,

I have provided the opposite rule, fixed indentation and added a bunch of unit tests. Hope that helps.

@mdevils mdevils force-pushed the master branch 2 times, most recently from db9f580 to 2fa147c Compare August 27, 2014 00:48
@mikesherov
Copy link
Contributor

@markelog can you please review this one?

@markelog
Copy link
Member

Yep, will do that tomorrow

@markelog
Copy link
Member

@avishnyak please rebase with current master, fix the lint errors that will come up after rebase and i'm gonna land this

… the keyword is the first expression in a block.
@avishnyak
Copy link
Author

@markelog Should be good to go now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) when pulling 8e05b0a on avishnyak:master into 467a170 on jscs-dev:master.

@mikesherov
Copy link
Contributor

@markelog ping!

@markelog
Copy link
Member

markelog commented Oct 8, 2014

@avishnyak sorry it took that long, missed the letter with your comment.

One other thing, could these rules accept true value like all rules of JSCS with possible array value?

If value is true it would use this -

exports.spacedKeywords = [
array?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.77%) when pulling 3b41606 on avishnyak:master into 467a170 on jscs-dev:master.

@avishnyak
Copy link
Author

@markelog This should be good to go now.


```js
"requirePaddingNewlinesBeforeKeywords": [
"do",
Copy link
Member

Choose a reason for hiding this comment

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

Additional spacing?

@markelog
Copy link
Member

Could you add a test for true value for each rule?

@markelog
Copy link
Member

I finished up with your pull, thank you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants