-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: add fixer for newline-before-return (fixes #5958) #7050
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "code" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spaces are preferred in the little-known ESLint style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.