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

JSCS modifications to one-var #4680

Closed
confuser opened this issue Dec 12, 2015 · 33 comments · Fixed by Urigo/tortilla#62, #9994, yhirano55/ama#57, annict/annict#1039 or dependabot/dependabot-core#312
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

@confuser
Copy link

Starting to migrate from JSCS/JSHint and I was looking for the equivalent of requireMultipleVarDecl

Unfortunately one-var doesn't cover this, as it prevents multiple variable declarations in a single scope. I'm looking for allowing multiple variables throughout the scope, but preventing consecutive var declarations.

Invalid:

var a = 0
var b = 1
var c = 2

console.log('hello')

var d
var e

Valid:

var a = 0
  , b = 1
  , c = 2

console.log('hello')

var d, e

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot
Copy link

@confuser Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Dec 12, 2015
@nzakas
Copy link
Member

nzakas commented Dec 12, 2015

It sounds like this would have to be an additional option for one-var, just not sure what the option name would be.

@nzakas nzakas 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 Dec 12, 2015
@confuser
Copy link
Author

@nzakas what about setting the option to multi?

E.g.

"one-var": [2, {
    "var": "multi",
    "let": "multi",
    "const": "multi"
}]

"one-var": [2, "multi"]

@nzakas
Copy link
Member

nzakas commented Dec 12, 2015

"Multi" is pretty ambiguous in this context because it's the opposite of one (and we already have "always" and "never")

@platinumazure
Copy link
Member

What about something like "non-consecutive" or "non-consecutive-only"?

@SpadeAceman
Copy link

It sounds like the main request here is for a separate rule to allow or prevent multiple separate var/let/const statements in sequence. (combine-seq-var-declarations perhaps?)

This rule would enforce or prevent a single var/let/const statement for sequential variable declarations, regardless of the location of those var/let/const statements. (In other words, this seems like a rule that ought to be independent of one-var's scope-related concerns.)

Valid when never specified (sequential variable declarations are never combined together):

var a = 0
var b = 1
var c = 2

console.log('hello')

var d
var e

Valid when always specified (sequential variable declarations are always combined together):

var a = 0
  , b = 1
  , c = 2

console.log('hello')

var d, e

@nzakas
Copy link
Member

nzakas commented Dec 29, 2015

It can't be a separate rule because it will clash with one-var.

@confuser
Copy link
Author

@nzakas Can this be added to the JSCS Compatibility milestone as part of #5856?

@ilyavolodin ilyavolodin added this to the JSCS Compatibility milestone Apr 16, 2016
@nzakas
Copy link
Member

nzakas commented Apr 16, 2016

We still need a concrete proposal for this rule.

@qfox
Copy link

qfox commented Apr 26, 2016

Repeating here to close #5910 as dup:

requireMultipleVarDecl, disallowMultipleVarDeclhttp://eslint.org/docs/rules/one-var (?)

onevar doesn't cover atm these cases:

  • {requireMultipleVarDecl: true} — it's what this issue is about
  • {requireMultipleVarDecl: {allExcept:["require"]}} — allows each require to have separate var (?)
  • {disallowMultipleVarDecl: {allExcept:["require"]}} — requires each variable to have separate var except require
  • {disallowMultipleVarDecl: {allExcept:["undefined"]}} (if it's not the same as {var: {uninitialized: "always"}}) — requires each variable separate except undefined ones (var a, b, c;)

Current one-var options: { var | let | const | uninitialized | initialized: "always"|"never" }.

@mysticatea
Copy link
Member

mysticatea commented May 9, 2016

Now, I have a question.

var a = 0
  , b = 1
  , c = 2

console.log('hello')

var d, e  // Why 'one-var' rule is warning this declaration?

I feel it's the responsibility of vars-on-top rule.

image

@ilyavolodin
Copy link
Member

@mysticatea I think it's responsibility of both rules. one-var should warn you that there are multiple var keywords used, and vars-on-top should warn that variable declaration is not at the begging of the block.
I think your screenshot actually looks correct.

@mysticatea
Copy link
Member

mysticatea commented May 10, 2016

Doesn't one-var which warns only neighbor declarations work fine?
vars-on-top indicates developers to move declarations to the neighbor of the first declaration.

@ilyavolodin
Copy link
Member

ilyavolodin commented May 10, 2016

@mysticatea one-var in it's default configuration basically requires you to only have 1 var keyword per file/function/class. vars-on-top doesn't care how many var keywords you have, as long as they all are located at the beginning of the block.

@mysticatea
Copy link
Member

mysticatea commented May 10, 2016

My question is, why one-var is "per file/function"? It sounds fine that a check of only neighbor declarations is. The rest part seems to be overlapping with vars-on-top...

@mysticatea
Copy link
Member

mysticatea commented May 10, 2016

My proposal is

  • Adds "neighbor" option value.
  • Adds 2nd optional object {"require": value} to each pattern.

This avoids breaking changes.

one-var

{
    "one-var": [
        "error", 
        "always" or "never" or "neighbor",
        {
            "require": "always" or "never" or "neighbor"
        }
    ],
    // or
    "one-var": [
        "error", 
        {
            "initialized": "always" or "never" or "neighbor",
            "uninitialized": "always" or "never" or "neighbor"
        },
        {
            "require": "always" or "never" or "neighbor"
        }
    ],
    // or
    "one-var": [
        "error", 
        {
            "var": "always" or "never" or "neighbor",
            "let": "always" or "never" or "neighbor",
            "const": "always" or "never" or "neighbor"
        },
        {
            "require": "always" or "never" or "neighbor"
        }
    ]
}
  • "always" (default) - keep the current behavior as is.
  • "never" - keep the current behavior as is.
  • "neighbor" - one-var warns neighbor variable declarations. This is the same behavior as JSCS rules.
  • require (default is no overriding) - This is the setting for retuire(). JSCS rules have this option.

/*eslint one-var:["error", "neighbor"]*/

//----------------------------------------------------------------
// Good
let a = 0,
    b = 1;

foo();

let c = 2;

//----------------------------------------------------------------
// Bad
let a = 0,
    b = 1;
let c = 2;
/*eslint one-var:["error", "never", {"require": "always"}]*/

//----------------------------------------------------------------
// Good
let a = require("a"),
    b = require("a");

let c = 2;
let d = 3;

//----------------------------------------------------------------
// Bad
let a = 0,
    b = 1;
let c = require("c");
let d = require("d");

@ilyavolodin
Copy link
Member

@mysticatea Basically one-var with always configuration says "always combine all of the variable declarations into a single declaration". And never is the reverse of that. Having a single var keyword per file/function is a pretty common pattern. We could add those two properties for JSCS compatibility, but the default configuration should stay the same. I don't think there's an overlap with vars-on-top at all. one-var controls number of var keywords, and vars-on-top controls location of variable declarations. You could enable one-var and put all of the variable declarations at the bottom of the function, for example, and one-var would be perfectly fine.

@mysticatea
Copy link
Member

mysticatea commented May 10, 2016

hmmm, I think the pretty common pattern is the combination of one-var and vars-on-top. not one-var only.

@mysticatea
Copy link
Member

Anyway, I don't like breaking changes.
So my proposal here is above.

@nzakas nzakas changed the title Rule: Grouped var JSCS modifications to one-var May 15, 2016
This was referenced Mar 22, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 13, 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 Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.