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
Add auto-fixing to newline-before-return #5958
Comments
Go for it |
Just one thing to be aware of: adding a line break means we need to know what the line break style is. Although, with multipass, that can be fixed on the next pass. Nevermind! :) |
Yes, that's the intention. We don't need to know that anymore. :) |
I'll table this for now in favor of JSCS compatibility issues. If anyone else is interested in picking this up, please feel free. |
@kaicataldo thanks for pinging me, I'd certainly be interested :) |
@vitorbal Any time :) |
@vitorbal Pinging here to see how things are going |
I am not sure if this kind of fixes (assuming the |
Another problem i encountered is that: If there's a comment on a line just above the return statement, then I dont On Sun, May 29, 2016 at 7:44 AM alberto notifications@github.com wrote:
|
Hm, yeah. The rule has also changed in the mean time, meaning the more clear-cut fix removing extra newlines is no longer necessary. Perhaps we should just close this. |
@eslint/eslint-team what do you guys think? |
IMO, the newline style, and tab style are both "top level" configuration options. I know we currently don't have this concept yet, but as we delve further and further into autofixing, we're going to see the need for it if we want individual rules to "do the right thing" in the absense of other rules that strictly require knowledge of newline style. To that effect, I would propose that the internal API we use for autofixing should acquire an |
Agree with @mikesherov, would be great if we somehow support this. |
+1 for auto-fix newline-before-return |
@mikesherov @zxqfox We discussed this before. However, now that we have multi-pass autofixing, I'm not sure we need it anymore. If you replace newline with just |
@ilyavolodin that's assuming the user is using the |
@ilyavolodin @mikesherov @zxqfox I have revived my proposal for specifying shared repository settings in configuration: #6298. I used |
Because the comment can be directly related to the return statement and some style guides would have them stay together. Take the following example: function foo() {
var bar = doSomething();
// return the result
return bar;
} We can fix it in the following two ways: function foo() {
var bar = doSomething();
// return the result
return bar;
} function foo() {
var bar = doSomething();
// return the result
return bar;
} I don't know if it's safe to assume either of them is the correct fix for all cases. |
Ah, I though you about this case: function foo() {
this.is();
// The end.
} I've misread issue title, sorry. In your case I'd stick comments to the next statement. Actually, we can add options to tune it up but does it really need someone? |
The gold standard for rule fixing is this: if the rule would flag the code as a problem, then you can safely fix it. Did the rule currently flag a |
@nzakas The rule would not flag these: function a() {
if (b) {
c();
}
//comment
return b;
} function a() {
if (b) {
c();
}
//comment
return b;
} But would flag this: function a() {
if (b) {
c();
}
//comment
return b;
} |
Correct - the rule subtracts the number of comment lines from the total to calculate it, but it doesn't make any assumptions about/enforce a standard for where the line break is |
Is the fix done? |
@cristian-sima the fix is unfortunately still not done. We haven't decided what to do regarding the discussion above, and if relying on the multi-pass is good enough. |
I am waiting for this fix to be done |
@cristian-sima please just subscribe to this issue rather than leaving additional comments. When the fix has been checked in, the issue will be closed and you'll get notified. |
@eslint/eslint-team Doesn't seem like we're really making any progress on this. I know it's accepted, but am wondering if we should close it? |
Looking back at my comment here, do you think it would suffice to assume that inserting the newline before the comment is an acceptable fix (the first fixed example in that linked comment)? Otherwise, I am still not sure how to determine that programmatically. |
@vitorbal That makes sense to me. I think we have to make an assumption one way or another and that seems like the style I see more often i.e.
|
Folks-- don't forget that we can always choose not to emit a fixer on a per-lint error basis if we don't think we can do it safely for a particular lint violation. |
@platinumazure very true! So an alternative would be that we just don't do a fix if we detect a comment just above the return statement. |
It's too bad we don't have an auto fix for lines-around-comment, or we could rely on multi pass fixing and just go ahead and fix everything. I could take a look and see if I can implement that as a prerequisite to this issue. Otherwise, given that we don't have that yet, I think @platinumazure's reminder is a good one and we could just not fix that case. Fixing some things is better than not fixing anything! |
Okay, agreed. Thanks for the input everyone! I will work on this most likely during the weekend. |
Quick update: I'll probably have a PR for this by the weekend! |
Fantastic! |
Following @alberto's suit.
If accepted, I'd like to implement this.I'll table this for now in favor of JSCS compatibility issues. If anyone else is interested in picking this up, please feel free.Some interesting challenges I found while doing some initial work on this:
return
statement and the previous token, I think that the newline should come after the previous token (but before the comment), as this seemed to me to be the more often used case. This shouldn't be the behavior if the comment is on the same line as the previous token, though (in which case the newline should probably come after the comment).Alternatively, if we feel like making those decisions is overreaching for a stylistic rule like this, then we can just not auto-fix if there are comments between the
return
statement and the previous token.The text was updated successfully, but these errors were encountered: