Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [explicit-member-accessibility] fix parameter properties bugs #912

Merged
merged 6 commits into from Sep 2, 2019
Merged

fix(eslint-plugin): [explicit-member-accessibility] fix parameter properties bugs #912

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
257683c
fix(eslint-plugin): [explicit-member-accessibility] bugs
ankeetmaini Aug 27, 2019
1f10ff2
fix(eslint-plugin): remove constructor check
ankeetmaini Aug 30, 2019
e7016c9
fix(eslint-plugin): handle readonly in pram prop
ankeetmaini Aug 31, 2019
a366eaa
fix(eslint-plugin): add readme for paramProp
ankeetmaini Aug 31, 2019
cbe3837
Merge branch 'master' into fix-member-accessibility
ankeetmaini Aug 31, 2019
a81bb21
Merge branch 'master' into fix-member-accessibility
bradzacher Sep 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 35 additions & 2 deletions packages/eslint-plugin/src/rules/explicit-member-accessibility.ts
View file
Open in desktop
Expand Up @@ -26,6 +26,19 @@ type MessageIds = 'unwantedPublicAccessibility' | 'missingAccessibility';

const accessibilityLevel = { enum: ['explicit', 'no-public', 'off'] };

const isParentConstructor = (node: TSESTree.TSParameterProperty) => {
let parent = node.parent;
while (parent) {
if (parent.type === 'MethodDefinition' && parent.kind === 'constructor') {
return true;
}
// go up
parent = parent.parent;
}

return false;
};

export default util.createRule<Options, MessageIds>({
name: 'explicit-member-accessibility',
meta: {
Expand Down Expand Up @@ -190,14 +203,34 @@ export default util.createRule<Options, MessageIds>({
return;
}

if (ctorCheck === 'off' && isParentConstructor(node)) {
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
return;
}

const nodeName =
node.parameter.type === AST_NODE_TYPES.Identifier
? node.parameter.name
: // has to be an Identifier or TSC will throw an error
(node.parameter.left as TSESTree.Identifier).name;

if (paramPropCheck === 'no-public' && node.accessibility === 'public') {
reportIssue('unwantedPublicAccessibility', nodeType, node, nodeName);
switch (paramPropCheck) {
case 'explicit': {
if (!node.accessibility) {
reportIssue('missingAccessibility', nodeType, node, nodeName);
}
break;
}
case 'no-public': {
if (node.accessibility === 'public') {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this should also check for readonly.
public foo is valid (otherwise it's not a parameter property), but public readonly foo is not

Suggested change
if (node.accessibility === 'public') {
if (node.accessibility === 'public' && node.readonly) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked public readonly foo in ASTExplorer using @typescript-eslint/parser, it showed that it is a valid AST.

https://astexplorer.net/#/gist/462492e71de4506a8ece6dbd055d2196/4273aa53fae3642835f358371cda2fe08e350bed

Also if I were to add that check, if parameterProperties='no-public', then public foo: string will pass as readonly is missing but still the access modifier is public.

Copy link
Member

Choose a reason for hiding this comment

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

For explicit:
public foo and public readonly foo are both considered valid (obviously!)

For no-public:
public foo is considered valid, because without public, it's not a parameter prop.

public readonly foo is considered invalid, because you can remove the accessibility modifier, and it's still a parameter property: readonly foo.

We should probably document this in the readme so that it's a bit clearer, as I don't think that this is entirely intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've incorporated these changes, thanks a lot for putting these down. HAve added in the readme too :)

reportIssue(
'unwantedPublicAccessibility',
nodeType,
node,
nodeName,
);
}
break;
}
}
}

Expand Down
184 changes: 184 additions & 0 deletions packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts
View file
Open in desktop
Expand Up @@ -10,6 +10,104 @@ ruleTester.run('explicit-member-accessibility', rule, {
{
filename: 'test.ts',
code: `
class Test {
public constructor(private foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'explicit' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(private readonly foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'explicit' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(private foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'off' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(protected foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'off' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(public foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'off' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(readonly foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'off' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(private readonly foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'off' },
},
],
},
{
filename: 'test.ts',
code: `
class Test {
protected name: string
private x: number
Expand Down Expand Up @@ -147,11 +245,97 @@ class Test {
},
],
},
{
filename: 'test.ts',
options: [
{ accessibility: 'no-public', overrides: { constructors: 'off' } },
],
code: `
class Foo {
constructor(public bar) {}
}
`,
},
{
filename: 'test.ts',
code: `
export class XXXX {
public constructor(readonly value: string) {}
}
`,
options: [
{
accessibility: 'off',
overrides: {
parameterProperties: 'explicit',
},
},
],
},
{
filename: 'test.ts',
code: `
export class XXXX {
private constructor(readonly samosa: string) {}
}
`,
options: [
{
accessibility: 'off',
overrides: {
constructors: 'no-public',
},
},
],
},
],
invalid: [
{
filename: 'test.ts',
code: `
export class WithParameterProperty {
public constructor(readonly value: string) {}
}
`,
options: [{ accessibility: 'explicit' }],
errors: [{ messageId: 'missingAccessibility' }],
},
{
filename: 'test.ts',
code: `
export class XXXX {
public constructor(readonly samosa: string) {}
}
`,
options: [
{
accessibility: 'off',
overrides: {
constructors: 'explicit',
parameterProperties: 'explicit',
},
},
],
errors: [{ messageId: 'missingAccessibility' }],
},
{
filename: 'test.ts',
code: `
class Test {
public constructor(readonly foo: string) {}
}
`,
options: [
{
accessibility: 'explicit',
overrides: { parameterProperties: 'explicit' },
},
],
errors: [{ messageId: 'missingAccessibility' }],
},
{
filename: 'test.ts',
code: `
class Test {
x: number
public getX () {
Expand Down