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

Implement fix for noUnnecessaryElseRule #103

Merged
merged 1 commit into from
Jan 3, 2019
Merged

Conversation

eile
Copy link
Contributor

@eile eile commented Dec 21, 2018

No description provided.

Copy link
Owner

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this PR.
I left some comments regarding the implementation.
Incidentally the thing with block-scoped variables in the else-block is also discussed and fixed in eslint/eslint#11076

The test need to be updated for the changed error output (see failed CI of this PR). Also please add a .fix file next to the .lint file to ensure the fixes work as expected.

@eile eile force-pushed the master branch 2 times, most recently from 0450bb4 to 8b05c1a Compare January 3, 2019 10:05
@eile
Copy link
Contributor Author

eile commented Jan 3, 2019

I've decided to remove the stripping of the else block, since I couldn't figure out a way to do this sanely in a single fix. It almost feels like it should be a separate no-unneccassary-block rule with fixer.

@eile
Copy link
Contributor Author

eile commented Jan 3, 2019

Just saw your comments, reading now..

@eile
Copy link
Contributor Author

eile commented Jan 3, 2019

Ah, much better now. Thanks for the explanations.

Copy link
Owner

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

the changes look good to me. I just added comments for minor improvements.

return ts.forEachChild(sourceFile, cb);
}

/** Return true to traverse children, false otherwise */
Copy link
Owner

Choose a reason for hiding this comment

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

this comment no longer applies

}
}

class IfWalker extends Lint.AbstractWalker<void> {
Copy link
Owner

Choose a reason for hiding this comment

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

could be reverted to extend AbstractIfStatementWalker. you can then drop the implementation of walk

Copy link
Owner

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

Thanks @eile

I'll publish a new release today so you can enjoy this new fixing capability.

@ajafff ajafff merged commit b00d6e8 into ajafff:master Jan 3, 2019
@ajafff
Copy link
Owner

ajafff commented Jan 3, 2019

And it's published as v1.15.0.
I also reused the logic for error reporting and fixing for no-else-after-return.

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.

2 participants