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

no-duplicate-variable: handle parameters #2597

Merged
merged 4 commits into from May 31, 2017
Merged

no-duplicate-variable: handle parameters #2597

merged 4 commits into from May 31, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Apr 18, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

[new-rule-option] no-dupliate-variable adds check-parameters to check if variable has the same name as a parameter

This is needed because of the behavior change of no-shadowed-variable in my upcoming PR.
Also the name of the rule implies this behavior. At least I expected that...

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

The rule does not check for duplicate parameter names, because the compiler already does that.

Changelog entry

[new-rule-option] no-duplicate-variable adds check-parameters to check if variable has the same name as a parameter

@adidahiya
Copy link
Contributor

please merge develop, this will fail strict-boolean-expressions #2408

[enhancement] `no-dupliate-variable`: added check if variable has the same name as a parameter
@ajafff
Copy link
Contributor Author

ajafff commented May 11, 2017

@adidahiya I can't confirm there is anything failing. Anyways, I rebased on master.

@@ -108,6 +109,7 @@ function testArguments1(arg: number, arg: number): void {
// failure: local var/let declarations shadow arguments.
function testArguments2(x: number, y: number): void {
var x = 1;
~ [err % ('x')]
let y = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't y a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this rule doesn't handle block scoped declarations. these result in a compiler error, so there is no need for a linter error

@adidahiya
Copy link
Contributor

hmm, I dunno about this... parameters do feel a little different from var declarations in the function body, which is why I put this check into no-shadowed-variable... is it really worth the breaking change? can you make the enhancements in #2598 without making this change @ajafff?

@ajafff
Copy link
Contributor Author

ajafff commented May 24, 2017

@adidahiya I don't think having this check in no-shadowed-variable is the right thing to do. no-shadowed-variable allows declarations with the same name in the same scope. They either merge (e.g. interface and class) or give a compiler error (e.g. two block scoped variables or block scoped variable and class). Parameters and variables merge if types match, so adding this additional check would be an ugly hack.

How about adding a new config option "check-parameters" to this rule to make it non-breaking?

@ajafff
Copy link
Contributor Author

ajafff commented May 30, 2017

@adidahiya I added a new options "check-parameters" to make this change non-breaking

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.

None yet

3 participants