Update: add fixer for newline-before-return (fixes #5958) #7050
Conversation
|
@vitorbal, thanks for your PR! By analyzing the annotation information on this pull request, we identified @kaicataldo, @alberto and @mysticatea to be potential reviewers |
|
LGTM |
| @@ -16,6 +16,8 @@ module.exports = { | |||
| recommended: false | |||
| }, | |||
|
|
|||
| fixable: "whitespace", | |||
There was a problem hiding this comment.
Should this be "code" instead?
There was a problem hiding this comment.
I think this should be "whitespace" since we're only altering whitespace
|
In retrospect, I feel like newline-before-return should have enforced a newline before return statements OR, if preceded by a comment, a newline before the comment. When I wrote it I was trying to make it not care about the location of the newline (before or after the comment, if there was one). Edit: Remembering why we did that now - it was to make it compatible with |
|
@kaicataldo, yeah, I think that would have been most likely okay from a user perspective. But you know what they say about hindsight :D |
|
LGTM |
| function hasNewlineBefore(node) { | ||
| const lineNumNode = node.loc.start.line; | ||
| const lineNumTokenBefore = getLineNumberOfTokenBefore(node); | ||
|
|
There was a problem hiding this comment.
Looks like an extra newline here
|
This is looking really good to me - one thought about making the |
|
LGTM |
|
@kaicataldo I pushed a new commit that makes the fixer a bit smarter when it comes to the cases that would previously require a multipass to fix. |
| * Setting lineNumTokenBefore to zero in that case results in the | ||
| * desired behavior. | ||
| */ | ||
| * Global return (at the beginning of a script) is a special case. |
There was a problem hiding this comment.
Looks like these spaces were removed, but I'm actually used to seeing them there. The examples in the valid-jsdoc docs also follow this style.
There was a problem hiding this comment.
The spaces are preferred in the little-known ESLint style guide.
There was a problem hiding this comment.
oh, d'oh! I'm not sure how they got removed, was definitely not intentional :) will fix it!
|
We should update the docs to note that this is now fixable |
|
Two small comments, but otherwise LGTM. Nice work on this! |
|
LGTM |
|
LGTM. Thanks |
eslint-deprecated
bot
added
the
archived due to age
What issue does this pull request address?
#5958 Add auto-fixing for "newline-before-return" rule.
What changes did you make? (Give an overview)
Added a fixer for all the cases that can be safely fixed, i.e. when the return statement does not have a comment attached to it.
I did some refactorings so I could re-use the logic that the rule already uses to calculate the number of preceding lines of a given return statement. The fixer uses that same logic to determine if the return statement is safe to fix or not.
Is there anything you'd like reviewers to focus on?
I added a couple of questions as comments in the diff. Otherwise, nothing in particular.