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

Allow padding on simple returns for newline-before-return #6176

Closed
andrewvaughan opened this issue May 14, 2016 · 14 comments
Closed

Allow padding on simple returns for newline-before-return #6176

andrewvaughan opened this issue May 14, 2016 · 14 comments
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@andrewvaughan
Copy link

ESLint Version: 2.10.1

Problem:

Using newline-before-return with padded-blocks causes a conflict, as padded-blocks insists on padded spacing for functions, and newline-before-return complains if there is a newline before a return statement by itself in a function.

Recommended Solution:

Add an option to newline-before-return to ignore scenarios where the only statement in a function block is the return statement.

For example:

"newline-before-return": ["error", {
    "pad-getters": true  // (New) Ensure getter returns are padded on either side by a newline
}]

This will allow ESLint to properly support it's built-in plugins without changing the default functionality of the newline-before-return rule. With the above configuration:

This code would result in an error:

function getVar() {
    return this.var;
}

This code would be valid with both plugins:

function getVar() {

    return this.var;

}
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label May 14, 2016
@alberto alberto added bug ESLint is working incorrectly 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 May 14, 2016
@alberto
Copy link
Member

alberto commented May 14, 2016

I think newline-before-return should not error when it is the first statement in a block, regardless of the newlines and that should be left to padded-blocks.

The doc says (emphasis mine):

This rule requires an empty line before return statements to increase code clarity, except when the return is alone inside a statement group (such as an if statement). In the latter case, the return statement does not need to be delineated by virtue of it being alone.

@kaicataldo
Copy link
Member

kaicataldo commented May 14, 2016

I support @alberto's suggested change 👍, though this would be a breaking change to the rule. I wonder if we should make it an option that relaxes the rule for that case?

@mysticatea mysticatea 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 May 17, 2016
@mysticatea
Copy link
Member

I added accepted label since there are 4 👍s from core team.

@alberto
Copy link
Member

alberto commented May 17, 2016

There is still the question of whether this is a breaking change and it should instead be treated as an enhancement go behind an option @eslint/eslint-team

There are a good number of tests asserting this behaviour

@mysticatea
Copy link
Member

I think we can modify the default behavior because this change is relaxing, so it probably will not break existing CI tests of users.

@mikesherov
Copy link
Contributor

In JSCS, relaxing rules was not considered a breaking change.

@markelog
Copy link
Member

Can we document what should be considered as breaking change? See JSCS example https://github.com/jscs-dev/node-jscs/blob/master/OVERVIEW.md#user-content-versioning--semver for reference

@nzakas
Copy link
Member

nzakas commented May 23, 2016

I concur that this is a bug as the documentation seems to indicate that the desired behavior.

I've opened an issue so we can discuss formalizing a semver policy: #6244

@nzakas nzakas closed this as completed in ee0cd58 May 23, 2016
@kalisjoshua
Copy link

I like this as an option. When should this be available? It looks like it is already included but I am not seeing it working.

@kaicataldo
Copy link
Member

Hey there, this should be in as of >2.11.0 http://eslint.org/blog/2016/05/eslint-v2.11.0-released#bug-fixes

@kalisjoshua
Copy link

@kaicataldo That's what I was thinking but I am not able to get this option to work; I still get:

error  Block must not be padded by blank lines  padded-blocks

... in my lint results output.

My .eslintrc.json looks like this:

{
  "env": {
    "browser": true,
    "node": true
  },
  "extends": "airbnb",
  "rules": {
    "comma-dangle": ["warn", "never"],
    "consistent-return": "error",
    "eqeqeq": ["error", "smart"],
    "newline-before-return": "off",
    "no-use-before-define": ["error", {
      "functions": false
    }],
    "space-before-function-paren": ["warn", "always"]
  }
}

I've tried all permutation of newline-before-return and padded-blocks. The only thing that seems to be working is padded-blocks which affects everything and I only want to allow an additional line before return statements. Am I understanding this correctly?

@kaicataldo
Copy link
Member

Ah yeah, that's the padded-blocks rule warning, not the newline-before-return rule. I recommend making a new issue for this, since this issue is unrelated and closed. Please make sure you include the info asked for in the template, including the code that's throwing the error. Thanks!

@kalisjoshua
Copy link

kalisjoshua commented Jun 9, 2016

@kaicataldo Correct, it is the padded-blocks error message but shouldn't the option proposed by this feature relax that? Am I misunderstanding the purpose of this additional option?

update: Here is a repo I put together to illustrate the issue - https://github.com/kalisjoshua/eslint-bug-example.

@kaicataldo
Copy link
Member

Looks like this fix was just for newlines before return statements. The padded-blocks warning is for the missing newline after the return statement (on line 4 in your example). The change in this issue makes the rules compatible by relaxing the newline-before-return rule to allow a newline before the return statement even when it's the only statement in the block. It sounds like you might not want to be using the padded-blocks rule, if you don't want a padding newline before the end of the block.

@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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

9 participants