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

Allow fast null checks in no-unused-expression rule #1638

Merged

Conversation

IllusionMH
Copy link
Contributor

This PR adds allow-fast-null-checks option for no-unused-expression rule which allows to use logical operators && and || to perform fast check for null(falsy) values and than call methods on non-null values or functions for side effects (e.g. e && e.preventDefault() in event handlers that for some reasons might be called without event object).

New functionality implemented as option to allow teams forbid fast null checks in their code in favor of more verbose implementations.

Implementation allows expression statements with && and || operators (may be nested or in parentheses) which has call expression in right most position , but doesn't checks if this call has side effects (it is developers duty).

@nchen63
Copy link
Contributor

nchen63 commented Nov 26, 2016

I'm a little uncomfortable with letting this syntax to creep into TypeScript, but I totally get how it's so much easier and that it's optional. @adidahiya?

@IllusionMH IllusionMH force-pushed the no-unused-expressions-fast-null-checks branch from aeef9b2 to 061d433 Compare January 7, 2017 00:20
@nchen63 nchen63 merged commit e649082 into palantir:master Jan 7, 2017
@adidahiya
Copy link
Contributor

@nchen63 seems ok if the behavior is is hidden behind a rule option. We probably won't enable it in the recommended config.

@IAfanasov
Copy link

PR waiting for review for 8 months. pity

@IllusionMH
Copy link
Contributor Author

It was merged half year ago. Only label left.

@gnapse
Copy link

gnapse commented Jun 20, 2018

This is documented to allow things like e && e.preventDefault(). But what about fn && fn()? It does not seem to allow that to pass. Is it intentional? I'd propose that that should be added too if it's not supported so far.

@suchanlee
Copy link
Contributor

@gnapse could you create an issue to track that? would welcome a PR for that!

@gnapse
Copy link

gnapse commented Jul 9, 2018

@suchanlee I created the issue (#4021) as requested. I'd love to work on a PR myself, but I'm totally new not just to tslint but to typescript in general. That's no reason to back off, but a hint on where to look for how to work on this would be greatly needed and appreciated. Would love to help!

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.

6 participants