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

no-shadowed-variable: add new option 'temporalDeadZone' #3389

Merged
merged 6 commits into from
Nov 28, 2017
Merged

no-shadowed-variable: add new option 'temporalDeadZone' #3389

merged 6 commits into from
Nov 28, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Oct 24, 2017

PR checklist

Overview of change:

Adds a new option to ignore variable shadowing when it occurs in the temporal dead zone of the shadowed block scoped variable.
Setting this option to true will restore the pre-v5.5.0 behavior, but only for block scoped declarations (parameters, classes, enums, let, const).
Fixes: #3078

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

CHANGELOG.md entry:

[new-rule-option] "temporalDeadZone" for no-shadowed-variable to ignore shadowing in the temporal dead zone of classes, parameters, enums and variables declared with let or const

Adds a new option to ignore variable shadowing when it occurs in the temporal dead zone of the shadowed block scoped variable.
Setting this option to `true` will restore the pre-v5.5.0 behavior, but only for block scoped declarations (parameters, classes, enums, let, const).
Fixes: #3078
@adidahiya adidahiya self-assigned this Oct 31, 2017
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

mostly looks good, small comments

@@ -35,6 +35,26 @@ export class Rule extends Lint.Rules.AbstractRule {
and \`"typeParameter"\`. Just set the value to \`false\` for the check you want to disable.
All checks default to \`true\`, i.e. are enabled by default.
Note that you cannot disable variables and parameters.

The option \`"temporalDeadZone"\` defaults to \`false\`. When set to \`true\` parameters, classes, enums and variables declared
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should default to true to align with the other options -- they are all true by default, which indicates that a certain type of check is enabled. Here, we intend to enable checking within temporal dead zones by default; requiring the option to be false would make sense to opt-out of the check -- this would make the rule less strict (similar to setting false on the other options).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you basically want me to swap true and false here and in all conditions? Sounds reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I would like you to swap true and false in all the new code you've added (leave the existing options as they were)


export function foo(wam: boolean) {
if (wam) {
const now = new Date();
Copy link
Contributor

Choose a reason for hiding this comment

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

would you mind marking this up with comments to indicate where the option is taking effect? similar to your example in the docs, just a simple // not an error with tdz disabled

@ajafff
Copy link
Contributor Author

ajafff commented Nov 28, 2017

Updated as suggested

@adidahiya adidahiya merged commit 2876450 into palantir:master Nov 28, 2017
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
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.

"no-shadowed-variable": vars shouldn't be shadowed by later declared vars
2 participants