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

Add auto-fixing to newline-before-return #5958

Closed
kaicataldo opened this issue Apr 25, 2016 · 42 comments
Closed

Add auto-fixing to newline-before-return #5958

kaicataldo opened this issue Apr 25, 2016 · 42 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@kaicataldo
Copy link
Member

kaicataldo commented Apr 25, 2016

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:

  • Keeping indentation (Not sure if necessary, given multi-pass auto-fixing, but seems like a nice thing to have)
  • If there's a comment between the 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.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Apr 25, 2016
@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 25, 2016
@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion accepted There is consensus among the team that this change meets the criteria for inclusion labels Apr 25, 2016
@alberto alberto added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 25, 2016
@alberto
Copy link
Member

alberto commented Apr 25, 2016

Go for it

@nzakas
Copy link
Member

nzakas commented Apr 25, 2016

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! :)

@alberto
Copy link
Member

alberto commented Apr 25, 2016

Yes, that's the intention. We don't need to know that anymore. :)

@kaicataldo
Copy link
Member Author

kaicataldo commented Apr 29, 2016

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
Copy link
Member Author

@vitorbal Here's an issue kind of similar to #5959, if you're interested :)

@vitorbal
Copy link
Member

@kaicataldo thanks for pinging me, I'd certainly be interested :)
Would it be okay if I bug you later with some questions once I have a chance to dig a bit deeper into some existing examples of auto-fixing?

@kaicataldo
Copy link
Member Author

@vitorbal Any time :)

@kaicataldo
Copy link
Member Author

@vitorbal Pinging here to see how things are going

@alberto
Copy link
Member

alberto commented May 29, 2016

I am not sure if this kind of fixes (assuming the linebreak-style rule is enabled) are a good idea anymore :/. What if it is not enabled because the user only develops on windows, so he doesn't think he needs that rule?

@vitorbal
Copy link
Member

Another problem i encountered is that:

If there's a comment on a line just above the return statement, then I dont
know how to determine if the newline should go above the comment or below
it.

On Sun, May 29, 2016 at 7:44 AM alberto notifications@github.com wrote:

I am not sure if this kind of fixes (assuming the linebreak-style rule is
enabled) are a good idea anymore :/. What if it is not enabled because the
user only develops on windows, so he doesn't think he needs that rule?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5958 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAmNdhm80J2FKw8PEazwpM7MAxICjc6eks5qGTXAgaJpZM4IOitB
.

@kaicataldo
Copy link
Member Author

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.

@vitorbal
Copy link
Member

@eslint/eslint-team what do you guys think?

@mikesherov
Copy link
Contributor

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 addNewline method that is aware of the style of newlines in the file (whether those are determined automatically by inspecting the current file or from a top level configuration).

@qfox
Copy link

qfox commented May 31, 2016

Agree with @mikesherov, would be great if we somehow support this.

@cristian-sima
Copy link

+1 for auto-fix newline-before-return

@ilyavolodin
Copy link
Member

@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 \n, next pass, linebreak-style will flag them as issues and autofix them. So in this particular case, we don't need shared styleguide.

@mikesherov
Copy link
Contributor

@ilyavolodin that's assuming the user is using the linebreak-style rule.

@platinumazure
Copy link
Member

@ilyavolodin @mikesherov @zxqfox I have revived my proposal for specifying shared repository settings in configuration: #6298. I used lines-around-comment as my example, but I will amend the proposal to show that newline-before-return could also benefit. Please do take a look and help me improve the proposal if it can be improved 😄

@vitorbal
Copy link
Member

vitorbal commented Jun 1, 2016

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.

@qfox
Copy link

qfox commented Jun 1, 2016

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?

@nzakas
Copy link
Member

nzakas commented Jun 1, 2016

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 areturn that has a comment on the previous line?

@vitorbal
Copy link
Member

vitorbal commented Jun 2, 2016

@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;
}

@kaicataldo
Copy link
Member Author

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

@vitorbal vitorbal self-assigned this Jun 7, 2016
@cristian-sima
Copy link

Is the fix done?

@vitorbal
Copy link
Member

@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.
One alternative would be having some shared repository settings, you can follow that discussion here: #6298

@cristian-sima
Copy link

I am waiting for this fix to be done

@nzakas
Copy link
Member

nzakas commented Jun 28, 2016

@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.

@kaicataldo
Copy link
Member Author

@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?

@vitorbal
Copy link
Member

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.

@kaicataldo
Copy link
Member Author

@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.

function fn() {
  if (something) {
    /* 
     * multi-line comment
     */
    return a; 
  }

  // comment
  return b;
}

@platinumazure
Copy link
Member

platinumazure commented Aug 25, 2016

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.

@vitorbal
Copy link
Member

@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.
@kaicataldo what do you think? Probably safer, although I also agree that the example in your last comment is the most obvious style used..

@kaicataldo
Copy link
Member Author

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!

@vitorbal
Copy link
Member

Okay, agreed. Thanks for the input everyone! I will work on this most likely during the weekend.

@vitorbal
Copy link
Member

vitorbal commented Sep 1, 2016

Quick update: I'll probably have a PR for this by the weekend!

@cristian-sima
Copy link

Fantastic!

vitorbal added a commit to vitorbal/eslint that referenced this issue Sep 3, 2016
vitorbal added a commit to vitorbal/eslint that referenced this issue Sep 4, 2016
vitorbal added a commit to vitorbal/eslint that referenced this issue Sep 7, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests