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.

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

check-if-else in prefer-consitional-expression #3

Closed
wants to merge 1 commit into from
Closed

check-if-else in prefer-consitional-expression #3

wants to merge 1 commit into from

Conversation

benoitdemaegdt
Copy link
Contributor

@benoitdemaegdt benoitdemaegdt commented Feb 21, 2018

The check-if-else option is kind of weird. I may be missing something, but this code is raising an error :

let level = '';
if (process.env.NODE_ENV === 'test') {
  level = 'emerg';
} else if (process.env.NODE_ENV === 'development') {
  level = 'debug';
} else {
  level = 'info';
}

According to this github issue : palantir/tslint#2899
The right way to do it (when using the check-if-else option) is to use ternary operators :

level = (process.env.NODE_ENV === 'test')
  ? 'emerg'
  : (process.env.NODE_ENV === 'development')
    ? 'debug'
    : 'info';

I do not like this syntax. The rule should only raise an error when there is only one else. Example :

if (process.env.NODE_ENV === 'test') {
  level = 'emerg';
} else {
  level = 'debug'
}

@benoitdemaegdt benoitdemaegdt added the question Further information is requested label Feb 21, 2018
@ccoeurderoy
Copy link
Contributor

ccoeurderoy commented Feb 21, 2018

Use a switch statement instead of if...else if...else:

let level = '';
const NODE_ENV = process.env.NODE_ENV;
switch (NODE_ENV) {
  case 'test':
  level = 'emerg';
  break;

  case 'development':
  level = 'debug';
  break;
  
  default:
  level = 'info';
  break;
}

No need to change this rule.

@benoitdemaegdt benoitdemaegdt added closed and removed question Further information is requested labels Feb 21, 2018
@benoitdemaegdt
Copy link
Contributor Author

use switch instead

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

Successfully merging this pull request may close these issues.

2 participants