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

Fix: Fix behavior of ignoring comments within previous nodes (refs #256) #262

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

kaicataldo
Copy link
Member

While adding tests to the main ESLint repo to account for the changes I made in #257 I realized that some of my initial assumptions were wrong:

  • That the body prop is always an array (in reality it can be an array or an object)
  • I didn't check the body props recursively (I had incorrectly thought the Program node's body contained all the nodes, which, in retrospect, is pretty silly)

This means that my current merged fix does not account for this case, as the comment to remove is nested a few levels deep, with the body props being a mixture of arrays and objects as you drill down:

function a() {
    var b = {
        // comment
    };
    return b;
}

I'm going to test out the main ESLint repo with this branch now, but wanted to see if I could get some eyes on this to see if it made sense. Thanks!

@kaicataldo
Copy link
Member Author

Tested by npm linking it to the main eslint repo. Was able to remove this guard (which is what alerted me to this problem in the first place!) and all the tests pass.

@nzakas
Copy link
Member

nzakas commented Mar 17, 2016

Lgtm

nzakas added a commit that referenced this pull request Mar 17, 2016
Fix: Fix behavior of ignoring comments within previous nodes (refs #256)
@nzakas nzakas merged commit 8ee7386 into eslint:master Mar 17, 2016
@kaicataldo kaicataldo deleted the refs256 branch April 18, 2016 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants