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

Update: no-inner-declarations false negative in non-block (fixes #12222) #13062

Merged
merged 10 commits into from Mar 31, 2020

Conversation

anikethsaha
Copy link
Member

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #12222

What changes did you make? (Give an overview)

Currently whenever the distance is more than 1, it was not showing error cause it was considering ancestor as any (Program included).
it will be valid only if ancestor is not Program for distance equal to 2

example

if(foo){
  var a;
}

this will result in distance 1 for the ancestor Program

if(foo)
  var a;

This will be distance 2 for the ancestor Program.
here both calculations for distance and ancestor are correct but not valid. (the master is considering it as valid)

So in this PR, whenever distance is 2, it should not check if from Program.

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

#12222

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 19, 2020
@anikethsaha anikethsaha changed the title Fix: consistencies for blockstmt no-inner-fn (fix #12222) Fix: consistent for blockstmt no-inner-fn (fix #12222) Mar 19, 2020
@anikethsaha anikethsaha changed the title Fix: consistent for blockstmt no-inner-fn (fix #12222) Fix: consistent for blockstmt no-inner-fn (fixes #12222) Mar 19, 2020
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 19, 2020
@mdjermanovic
Copy link
Member

These should be also allowed:

/*eslint no-inner-declarations: ["error", "both"]*/

export var foo;

export function bar() {}

export default function baz() {}

@anikethsaha
Copy link
Member Author

cc @mdjermanovic

I changed the validation condition, now its allowing those named and default exports

@mdjermanovic
Copy link
Member

Thoughts about simplifying the code?

A declaration is okay if its parent is either of:

  • Program
  • ExportNamedDeclaration
  • ExportDefaultDeclaration
  • BlockStatement which parent is either of FunctionDeclaration, FunctionExpression or ArrowFunctionExpression.

so maybe we could just check these simple conditions, instead of calculating the distance and then analyzing the result.

@anikethsaha
Copy link
Member Author

@mdjermanovic I simplified the code as per your suggestion and it is working as intended !

that while loop was needed as we need to find the correct immediate ancestor where the declaration should be moved for the error message.

let me know if it needs some more changes 👍

@mdjermanovic
Copy link
Member

that while loop was needed as we need to find the correct immediate ancestor where the declaration should be moved for the error message.

You're right! I missed the fact that the ancestor is used for the error message as well.

@anikethsaha
Copy link
Member Author

@mdjermanovic
should I revert the latest commit then? the latest commit gives the ancestor as expected as the old one though !

lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Looks great, I left just some minor suggestions.

We should also update the documentation with some examples.

lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

@mdjermanovic I updated the docs with some examples

docs/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

@mdjermanovic done 👍

@mdjermanovic mdjermanovic changed the title Fix: consistent for blockstmt no-inner-fn (fixes #12222) Update: no-inner-declarations false negative in non-block (fixes #12222) Mar 28, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! The change is implemented as intended.

Leaving the "needs bikeshedding" label since it isn't definitely confirmed that this should be the behavior of this rule.

@kaicataldo kaicataldo removed the needs bikeshedding Minor details about this change need to be discussed label Mar 30, 2020
@kaicataldo
Copy link
Member

Removing the "needs bikeshedding" label as per this comment.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@nzakas nzakas merged commit 85b7254 into eslint:master Mar 31, 2020
@anikethsaha anikethsaha deleted the fixes-12222 branch March 31, 2020 07:15
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 28, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-inner-declarations inconsistency
4 participants