[enhancement] member-access rule on parameter properties #3325

cyrilgandon · 2017-10-16T15:20:08Z

PR checklist

Addresses an existing issue: member-access rule should trigger with readonly parameter property #3323
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Check parameter property for rule member-access.

class myClass {
    constructor (
        readonly x: number, // <--- trigger member-access rule
        public readonly y: number, // <--- trigger member-access rule if no-public
    ) { }
}

Is there anything you'd like reviewers to focus on?

Do we need to treat this as an optional configuration, or should it be by default.
There is already no-public, check-accessor and check-constructor.
Does a check-parameter-property make sense, or just let the rule be by default like the other properties.

CHANGELOG.md entry:

[new-rule-option] check-parameter-property for member access rule

ajafff

I left some suggestions. Looks pretty good so far.

It's actually very nice that you thought about the edge case where public cannot simply be removed from parameter properties.

It's better to add a new option for this functionality. "check-parameter-property" should be fine (singular to keep it consistent with the other options)

ajafff · 2017-10-16T18:15:03Z

src/rules/memberAccessRule.ts

@@ -96,6 +96,13 @@ function walk(ctx: Lint.WalkContext<Options>) {
                }
            }
        }
+        if (isConstructorDeclaration(node)) {


you can move this check into the for...of loop above and use child instead of node. A ConstructorDeclaration can only occur in ClassLikeDeclaration.members.

You can also add && child.body !== undefined to avoid checking constructor overload signatures.

ajafff · 2017-10-16T18:15:59Z

src/rules/memberAccessRule.ts

@@ -96,6 +96,13 @@ function walk(ctx: Lint.WalkContext<Options>) {
                }
            }
        }
+        if (isConstructorDeclaration(node)) {
+            for (const param of node.parameters) {
+                if (ts.isParameterPropertyDeclaration(param)) {


prefer isParameterProperty from tsutils because it does a lot less work.

ajafff · 2017-10-16T18:18:59Z

src/rules/memberAccessRule.ts

@@ -159,6 +171,8 @@ function typeToString(node: ts.ClassElement): string {
            return "get property accessor";
        case ts.SyntaxKind.SetAccessor:
            return "set property accessor";
+        case ts.SyntaxKind.Parameter:
+            return "class parameter property";


I think you can omit "class" since parameter properties can only occur in classes.

ajafff · 2017-10-16T18:33:04Z

src/rules/memberAccessRule.ts

-            );
+            const readonlyKeyword = getModifier(node, ts.SyntaxKind.ReadonlyKeyword);
+            // public is not optional for parameter property without the readonly modifier
+            const isPublicOptional = !ts.isParameterPropertyDeclaration(node) || readonlyKeyword !== undefined;


you don't need to check for parameter properties here again. If a ts.ParameterDeclaration gets passed to this function, it's always a parameter property.
You can replace the method call with node.kind !== ts.SyntaxKind.Parameter

ajafff · 2017-10-16T18:36:59Z

also enable the new option in src/config/all.ts
and make sure to run yarn verify afterwards and fix the new failures in this project caused by these stricter checks

cyrilgandon · 2017-10-17T07:12:58Z

Great feedback, thank you!
There is no more error when I run yarn verify, because I added the rule option, and there is only "member-access": true, in the tslint.json, with a // TODO: Enable stricter options for these.
I'd be happy to enable those on member-access on a different PR.

ajafff · 2017-10-17T19:59:31Z

test/rules/member-access/parameter-property/test.ts.lint

@@ -0,0 +1,14 @@
+class Members {
+    constructor(
+        readonly parameterPropertyR : number,


please add a test with a decorator to make sure the public keyword is inserted at the correct position

nice idea, didn't think about decorators at all!

ajafff · 2017-10-17T20:01:53Z

test/rules/member-access/no-public/test.ts.lint

@@ -6,6 +6,9 @@ class {
    public/*some comment*/ get y() {}
    ~~~~~~ [0]
    public() {}
+    constructor(public readonly param : number) {}
+                ~~~~~~ [0]
+    constructor(public param : number) {}


add the same for private and protected, to make sure it won't regress

ajafff · 2017-10-17T20:04:55Z

src/rules/memberAccessRule.ts

@@ -15,7 +15,7 @@
 * limitations under the License.
 */

-import { getChildOfKind, getModifier, getNextToken, getTokenAtPosition, isClassLikeDeclaration } from "tsutils";
+import * as utils from "tsutils";


We usually tend to avoid namespace imports here. It's fine if the import list spans multiple lines.

Since this is only a matter of personal preference, I leave it up to you whether you want to change it or not.

ajafff · 2017-10-17T20:06:56Z

test/rules/member-access/no-public/test.ts.lint

@@ -6,6 +6,9 @@ class {
    public/*some comment*/ get y() {}
    ~~~~~~ [0]
    public() {}
+    constructor(public readonly param : number) {}


consider adding the public keyword to the constructor to make sure the rule finds both failures

cyrilgandon

I mistakenly pushes 2 files generated by yarn verify. It should not be part of this commit, but I guess it should be commited at one point. Would you like I revert those?

ajafff · 2017-10-18T14:06:30Z

Thanks a lot @cyrilgandon

The generated files are no problem. #3310 removes them from the git repository and ignores that folder.

Cyril Gandon added 2 commits October 16, 2017 15:48

member-access rule: check also for parameter property

a985353

member-access rule: add test for no-public option

1568dba

ajafff reviewed Oct 16, 2017

View reviewed changes

member-access rule: add new-rule-option check-parameter-property

ebce660

ajafff reviewed Oct 17, 2017

View reviewed changes

Cyril Gandon added 2 commits October 18, 2017 09:05

memberAccessRule: prefer multiline import to namespace import

6087fe8

memberAccessRule: add more tests

e5c4358

cyrilgandon commented Oct 18, 2017

View reviewed changes

ajafff approved these changes Oct 18, 2017

View reviewed changes

ajafff merged commit d197ee3 into palantir:master Oct 18, 2017

ajafff mentioned this pull request Oct 18, 2017

member-access rule should trigger with readonly parameter property #3323

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

[enhancement] member-access rule on parameter properties (palantir#3325)

7b4ff91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[enhancement] member-access rule on parameter properties #3325

[enhancement] member-access rule on parameter properties #3325

cyrilgandon commented Oct 16, 2017 •

edited

ajafff left a comment

ajafff Oct 16, 2017

ajafff Oct 16, 2017

ajafff Oct 16, 2017

ajafff Oct 16, 2017

ajafff commented Oct 16, 2017

cyrilgandon commented Oct 17, 2017 •

edited

ajafff Oct 17, 2017

cyrilgandon Oct 18, 2017

ajafff Oct 17, 2017

ajafff Oct 17, 2017

ajafff Oct 17, 2017

cyrilgandon left a comment

ajafff commented Oct 18, 2017

[enhancement] member-access rule on parameter properties #3325

[enhancement] member-access rule on parameter properties #3325

Conversation

cyrilgandon commented Oct 16, 2017 • edited

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

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

ajafff commented Oct 16, 2017

cyrilgandon commented Oct 17, 2017 • edited

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

cyrilgandon left a comment

Choose a reason for hiding this comment

ajafff commented Oct 18, 2017

cyrilgandon commented Oct 16, 2017 •

edited

cyrilgandon commented Oct 17, 2017 •

edited