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

fix no-duplicate-super validates with ternary operator #3544

Merged

Conversation

mateuszwitkowski
Copy link
Contributor

PR checklist

Overview of change:

Added case for ternary operator in no-duplicate-super rule.

CHANGELOG.md entry:

[bugfix] using ternary operator for calling super() now passes no-duplicate-super rule.

@@ -80,6 +80,11 @@ function walk(ctx: Lint.WalkContext<void>): void {
? { node: node.parent! as ts.CallExpression, break: false }
: Kind.NoSuper;

case ts.SyntaxKind.ConditionalExpression: {
const { whenTrue, whenFalse } = node as ts.ConditionalExpression;
return worse(getSuperForNode(whenTrue), whenFalse !== undefined ? getSuperForNode(whenFalse) : Kind.NoSuper);
Copy link
Contributor

Choose a reason for hiding this comment

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

whenFalse can't be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right of course. I'll fix it. There's another issue though. If someone would use nested ternary operators super() will be called twice:

	constructor(props?: any) {
        props ? super(props) : super() ? super() : super(); // if no props passed super() is called twice
	}
}

Do you think such a case should be covered? If so can you give me a tip how to handle it?

@@ -80,6 +80,11 @@ function walk(ctx: Lint.WalkContext<void>): void {
? { node: node.parent! as ts.CallExpression, break: false }
: Kind.NoSuper;

case ts.SyntaxKind.ConditionalExpression: {
const { whenTrue, whenFalse } = node as ts.ConditionalExpression;
return worse(getSuperForNode(whenTrue), getSuperForNode(whenFalse));
Copy link
Contributor

Choose a reason for hiding this comment

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

to also handle super() calls inside the condition:

const inCondition = getSuperForNode(node.condition);
const inBranches = worse(getSuperForNode(whenTrue), getSuperForNode(whenFalse));
if (typeof inCondition !== "number" && typeof inBranches !== number) {
    addDuplicateFailure(inCondition.node, inBranches.node);
}
return worse(inCondition, inBranches);

*code is untested and written only in GitHub comment editor.

@@ -259,5 +259,14 @@ declare const n: number;
}
}

// With ternary operator
{
class {
Copy link
Contributor

Choose a reason for hiding this comment

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

btw. this is not a valid class declaration. Class declarations need a name.

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 thought about it but that's how it's done in all the other test cases for that rule. I've figured out there's some reason for it and followed it. I'll fix it in all test cases with updated PR.

@ajafff ajafff merged commit ee501e4 into palantir:master Dec 5, 2017
@ajafff
Copy link
Contributor

ajafff commented Dec 5, 2017

Thanks @mateuszwitkowski

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

Successfully merging this pull request may close these issues.

2 participants