refactor newlineBeforeReturn to support more variants #3139

CSchulz · 2017-08-18T09:16:40Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Rename newlineBeforeReturn to newline and add some more options for adding newlines.

ajafff · 2017-08-18T19:05:10Z

I agree that it makes sense to rename and enhance the rule instead of adding yet another whitespace related rule. Nevertheless we need to keep newline-before-return around until the next major version to avoid a breaking change.
But that's pretty easy. Just export the walker of your new rule and use it from newline-before-return with the configuration to only require a blank line before return statements.
Also add a deprecation message to the metadata of newline-before-return

ajafff

Finished my first pass of the review. I only looked at the code. Will need to think a bit more about the configuration.

ajafff · 2017-08-18T19:08:37Z

src/rules/newlineRule.ts

+}
+
+const ALWAYS_IGNORE_OR_NEVER = {
+    enum: OptionState,


this is only used in the metadata? I don't think this works with string enums. It actually expects a string array here.

ajafff · 2017-08-18T19:11:18Z

src/rules/newlineRule.ts

+        options[optionName] = typeof json === "object" ? json[optionName] : json === undefined ? "always" : json;
+    }
+    return options;
+}


I'd suggest the following (note that this defaults to ignore, which might be more intuitive)

type Options = Record<OptionName, Option>; function parseOptions(json: Partial<Options> | undefined) { const default: Options = {return: "ignore", class: "ignore", functionBlock: "ignore", block: "ignore"}; return {...default, json}; }

ajafff · 2017-08-18T19:12:16Z

src/rules/newlineRule.ts

+        optionsDescription: Lint.Utils.dedent`
+            Following arguments may be optionally provided:
+
+            * \`"return"\` checks for empty line before return when not the only line in the block.


s/line/statement/

ajafff · 2017-08-18T19:13:24Z

src/rules/newlineRule.ts

+            Following arguments may be optionally provided:
+
+            * \`"return"\` checks for empty line before return when not the only line in the block.
+            * \`"class"\` checks for empty line after class declaration.


checks for empty line at the start of a class body

ajafff · 2017-08-18T19:13:38Z

src/rules/newlineRule.ts

+
+            * \`"return"\` checks for empty line before return when not the only line in the block.
+            * \`"class"\` checks for empty line after class declaration.
+            * \`"functionBlock"\` checks for empty line after function block declaration.


at the start of a function block

ajafff · 2017-08-18T19:26:32Z

src/rules/newlineRule.ts

+
+            if (isClassDeclaration(node) && this.options.class != OptionState.ignore) {
+                this.checkForEmptyLine(
+                    getChildOfKind(node, ts.SyntaxKind.OpenBraceToken, this.sourceFile)!, node.members[0],


instead of getChildOfKind(...) you could simply pass node.members.pos as first parameter. see my other comment in checkForEmptyLine below

ajafff · 2017-08-18T19:26:53Z

src/rules/newlineRule.ts

+                    (isFunctionWithBody(node.parent!) && this.options.functionBlock != OptionState.ignore
+                        || this.options.block != OptionState.ignore)) {
+                this.checkForEmptyLine(
+                    node.getChildAt(0), node.statements[0], this.options.functionBlock == OptionState.always, 'first statement',


instead of node.getChildAt(...) you could simply pass node.statements.pos as first parameter. see my other comment in checkForEmptyLine below

ajafff · 2017-08-18T19:27:17Z

src/rules/newlineRule.ts

+                    (isFunctionWithBody(node.parent!) && this.options.functionBlock != OptionState.ignore
+                        || this.options.block != OptionState.ignore)) {
+                this.checkForEmptyLine(
+                    node.getChildAt(0), node.statements[0], this.options.functionBlock == OptionState.always, 'first statement',


you need to check if the block contains any statement

ajafff · 2017-08-18T19:28:03Z

src/rules/newlineRule.ts

+    private checkForEmptyLine(node: ts.Node, firstNode: ts.Node, lineRequired: boolean, nodeType: string) {
+        let start = node.end;
+        let line = ts.getLineAndCharacterOfPosition(this.sourceFile, start).line;
+        let firstNodeStart = firstNode.getStart();


please provide the sourceFile parameter to getStart() for performance reasons

ajafff · 2017-08-18T19:31:46Z

test/rules/newline/none/test.tsx.lint

-
-[0]: Missing blank line before return
+
+


do these trailing whitespaces serve a purpose?

ajafff · 2017-09-27T16:21:31Z

src/rules/newlineRule.ts

+        optionsDescription: Lint.Utils.dedent`
+            Following arguments may be optionally provided:
+
+            * \`"return"\` checks for empty listane before return when not the only statement in the block.


s/listane/line/

ajafff · 2017-09-27T16:22:19Z

src/rules/newlineRule.ts

+            * \`"return"\` checks for empty listane before return when not the only statement in the block.
+            * \`"class"\` checks for empty line at the start of a class body.
+            * \`"functionBlock"\` checks for empty line at the start of a function block.
+            * \`"block"\` checks for empty line after block declaration.`,


s/after/in the first line of a/

ajafff · 2017-09-27T16:22:48Z

src/rules/newlineRule.ts

+
+export enum ViolationType {
+    needed = "needed",
+    needless = "needless",


prefer "unnecessary"

ajafff · 2017-09-27T16:27:49Z

src/rules/newlineRule.ts

+        optionsDescription: Lint.Utils.dedent`
+            Following arguments may be optionally provided:
+
+            * \`"return"\` checks for empty listane before return when not the only statement in the block.


as discussed offline: I don't like the option names.
I'd prefer "before-return" here.
I'm also in favor of "start-of-class" to allow adding "before-class" and "after-class" later on.

And to keep naming consistent, you should prefer "function-block" over "functionBlock"

ajafff · 2017-09-27T16:30:34Z

src/rules/newlineRule.ts

+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING_FACTORY(kind: ViolationType, nodeType: string) {
+        const kindMsg = kind === ViolationType.needed ? 'Missing' : 'Unneeded';


Why don't you use the string value of the enum?

CSchulz force-pushed the newline-rule branch from f6b28a0 to f2c3800 Compare August 18, 2017 09:20

refactor newlineBeforeReturn to support more variants

35c8d41

CSchulz force-pushed the newline-rule branch from f2c3800 to 35c8d41 Compare August 18, 2017 09:29

ajafff suggested changes Aug 18, 2017

View reviewed changes

first review comments implemented

0fb8b30

ajafff added the PR: waiting for reviewer label Sep 12, 2017

ajafff reviewed Sep 27, 2017

View reviewed changes

ajafff removed the PR: waiting for reviewer label Sep 27, 2017

ajafff mentioned this pull request Nov 28, 2017

Check number of empty lines between methods in classes #3517

Closed

adidahiya added the PR: waiting for author label Nov 28, 2017

jkillian added the Resolution: Aged Away label May 1, 2018

jkillian closed this May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor newlineBeforeReturn to support more variants #3139

refactor newlineBeforeReturn to support more variants #3139

CSchulz commented Aug 18, 2017

ajafff commented Aug 18, 2017

ajafff left a comment

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Aug 18, 2017

ajafff Sep 27, 2017

ajafff Sep 27, 2017

ajafff Sep 27, 2017

ajafff Sep 27, 2017

ajafff Sep 27, 2017

refactor newlineBeforeReturn to support more variants #3139

refactor newlineBeforeReturn to support more variants #3139

Conversation

CSchulz commented Aug 18, 2017

PR checklist

Overview of change:

ajafff commented Aug 18, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment