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

Rule Enhancement Proposal: indent and multiline parameters. #6052

Closed
mysticatea opened this issue May 3, 2016 · 14 comments
Closed

Rule Enhancement Proposal: indent and multiline parameters. #6052

mysticatea opened this issue May 3, 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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@mysticatea
Copy link
Member

From requireAlignedMultilineParams.

JSCS has the rule to align parameters vertically.
I think this is a responsibility of indent rule.

{
    "indent": ["error", 4, {
        "FunctionDeclaration": {
            "parameters": "first", // or an integer
            "body": 1
        },
        "FunctionExpression": {
            "parameters": "first", // or an integer
            "body": 1
        }
    }]
}

Those options are similar to existing SwitchCase and VariableDeclarator options.
But "parameters": "first" is special.
If "parameters": "first" is specified, 2nd line of parameters and later will be aligned with the same column as the first parameter item.

/*eslint indent: ["error", 4, {"FunctionDeclaration": {"parameters": "first"}}]*/

function foo(aaa, bbb,
             ccc, ddd) {
    doSomething();
}
@mysticatea mysticatea 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 labels May 3, 2016
@mysticatea mysticatea added this to the JSCS Compatibility milestone May 3, 2016
@doberkofler
Copy link
Contributor

+1

@BYK
Copy link
Member

BYK commented Jun 1, 2016

I also think this should be an option in indent rule.

@ghost
Copy link

ghost commented Jun 8, 2016

I also think it's important do implement this rule. Currently the sharable eslint-config-google has no way to enforce the parameter indentation proposed on Google JavaScript Style Guide:

When possible, all function arguments should be listed on the same line. If doing so would exceed the 80-column limit, the arguments must be line-wrapped in a readable way. To save space, you may wrap as close to 80 as possible, or put each argument on its own line to enhance readability. The indentation may be either four spaces, or aligned to the parenthesis.

The Google style also enforces double indentation on line-wrapped variable initializations and method chaining.

@BYK
Copy link
Member

BYK commented Jun 16, 2016

@eslint/doctrine-team we need three 👍s and a champion right? I can champion this and giving my 👍. 2 more to go I guess?

@mysticatea
Copy link
Member Author

👍 from me.

@nzakas
Copy link
Member

nzakas commented Jun 17, 2016

@BYK champion doesn't count as 👍, otherwise you'd just need two. :)

I'll give my 👍 , So with @mysticatea and @ilyavolodin , that's three.

@nzakas nzakas 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 Jun 17, 2016
@robations
Copy link

robations commented Jul 8, 2016

Does this proposal include non-aligned multiline parameters? I have lots of code similar to this:

angular
    .module(
        "zote",
        [ // A
            "sbor", // B
            "thed",
            "sneg"
        ] // C
    )
    .directive(...)
    .factory(...)
;

Currently eslint would expect the sample to have 4, 8 and 4 spaces for the lines A, B, C above, rather than 8, 12 and 8.

@mysticatea
Copy link
Member Author

@robations

  1. This proposal is about function declarations (not call expressions).
  2. I think inside of a parameter is indented normally.
function foo(a, b,
             [c],
             [
                 d,
                 e,
             ]
) {
    //
}

@robations
Copy link

@mysticatea

  1. yes sorry, I misread this, is it worth raising as a separate issue? I searched through the issue tracker for duplicate issues but I won't pretend to have grokked everything.
  2. Not sure I understand how this relates to my point, but yes.

@mysticatea
Copy link
Member Author

@robations Ah, ... Hmm, some similar codes have been posted to #4161, but #4161 seems about another problem. ..... I found #4696.

@not-an-aardvark
Copy link
Member

Working on this.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Sep 1, 2016

I wrote some tests, but haven't started the implementation yet.

Could someone double-check these tests to make sure there aren't any glaring errors? (There are probably some typos that I'll find when I implement the feature, but I mainly want to verify that the tests are roughly correct, to ensure that I understand the desired behavior correctly.)

// valid:
        {
            code:
            "function foo(aaa,\n" +
            "  bbb, ccc, ddd) {\n" +
            "    bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: 1, body: 2}}]
        },
        {
            code:
            "function foo(aaa, bbb,\n" +
            "      ccc, ddd) {\n" +
            "  bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: 3, body: 1}}]
        },
        {
            code:
            "function foo(aaa,\n" +
            "    bbb,\n" +
            "    ccc) {\n" +
            "            bar();\n" +
            "}",
            options: [4, {FunctionDeclaration: {parameters: 1, body: 3}}]
        },
        {
            code:
            "function foo(aaa,\n" +
            "             bbb, ccc,\n" +
            "             ddd, eee, fff) {\n" +
            "  bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: "first", body: 1}}]
        },
        {
            code:
            "function foo(aaa, bbb)\n" +
            "{\n" +
            "      bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {body: 3}}] // FIXME: what is the default for `parameters`?
        },
        {
            code:
            "function foo(\n" +
            "  aaa,\n" +
            "  bbb) {\n" +
            "    bar();\n" +
            "}",
            // edit: fixed body indentation for this case
            options: [2, {FunctionDeclaration: {parameters: "first", body: 2}}] // FIXME: make sure this is correct
        },
        {
            code:
            "var foo = function(aaa,\n" +
            "    bbb,\n" +
            "    ccc,\n" +
            "    ddd) {\n" +
            "bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: 2, body: 0}}]
        },
        {
            code:
            "var foo = function(aaa,\n" +
            "  bbb,\n" +
            "  ccc) {\n" +
            "                    bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: 1, body: 10}}]
        },
        {
            code:
            "var foo = function(aaa,\n" +
            "                   bbb, ccc, ddd,\n" +
            "                   eee, fff) {\n" +
            "    bar();\n" +
            "}",
            options: [4, {FunctionExpression: {parameters: "first", body: 1}}]
        },
        {
            code:
            "var foo = function(\n" +
            "  aaa, bbb, ccc\n" +
            "  ddd, eee) {\n" +
            "      bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: "first", body: 3}}] // FIXME: make sure this is correct
        }


// invalid:
        {
            code:
            "function foo(aaa,\n" +
            "    bbb, ccc, ddd) {\n" +
            "      bar();\n" +
            "}",
            output:
            "function foo(aaa,\n" +
            "  bbb, ccc, ddd) {\n" +
            "    bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: 1, body: 2}}],
            errors: expectedErrors([[2, 2, 4, "Identifier"], [2, 4, 6, "ExpressionStatement"]])
        },
        {
            code:
            "function foo(aaa, bbb,\n" +
            "  ccc, ddd) {\n" +
            "bar();\n" +
            "}",
            output:
            "function foo(aaa, bbb,\n" +
            "      ccc, ddd) {\n" +
            "  bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: 3, body: 1}}],
            errors: expectedErrors([[2, 6, 2, "Identifier"], [3, 0, 2, "ExpressionStatement"]])
        },
        {
            code:
            "function foo(aaa,\n" +
            "        bbb,\n" +
            "  ccc) {\n" +
            "      bar();\n" +
            "}",
            output:
            "function foo(aaa,\n" +
            "    bbb,\n" +
            "    ccc) {\n" +
            "            bar();\n" +
            "}",
            options: [4, {FunctionDeclaration: {parameters: 1, body: 3}}],
            errors: expectedErrors([[2, 4, 8, "Identifier"], [3, 2, 8, "Identifier"], [4, 12, 6, "ExpressionStatement"]])
        },
        {
            code:
            "function foo(aaa,\n" +
            "  bbb, ccc,\n" +
            "                   ddd, eee, fff) {\n" +
            "   bar();\n" +
            "}",
            output:
            "function foo(aaa,\n" +
            "             bbb, ccc,\n" +
            "             ddd, eee, fff) {\n" +
            "  bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: "first", body: 1}}],
            errors: expectedErrors([[2, 13, 2, "Identifier"], [3, 13, 19, "Identifier"], [4, 2, 3, "ExpressionStatement"]])
        },
        {
            code:
            "function foo(aaa, bbb)\n" +
            "{\n" +
            "bar();\n" +
            "}",
            output:
            "function foo(aaa, bbb)\n" +
            "{\n" +
            "      bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {body: 3}}],
            errors: expectedErrors([2, 6, 0, "ExpressionStatement"])
        },
        {
            code:
            "function foo(\n" +
            "aaa,\n" +
            "    bbb) {\n" +
            "bar();\n" +
            "}",
            output:
            "function foo(\n" +
            "  aaa,\n" +
            "  bbb) {\n" +
            "    bar();\n" +
            "}",
            options: [2, {FunctionDeclaration: {parameters: "first", body: 4}}],
            errors: expectedErrors([[2, 2, 0, "Identifier"], [3, 2, 4, "Identifier"], [4, 0, 4, "ExpressionStatement"]])
        },
        {
            code:
            "var foo = function(aaa\n" +
            "  bbb,\n" +
            "    ccc,\n" +
            "      ddd) {\n" +
            "  bar();\n" +
            "}",
            output:
            "var foo = function(aaa\n" +
            "      bbb,\n" +
            "      ccc,\n" +
            "      ddd) {\n" +
            "bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: 2, body: 0}}],
            errors: expectedErrors([[2, 4, 2, "Identifier"], [4, 4, 6, "Identifier"], [5, 0, 2, "ExpressionStatement"]])
        },
        {
            code:
            "var foo = function(aaa,\n" +
            "   bbb,\n" +
            " ccc) {\n" +
            "  bar();\n" +
            "}",
            output:
            "var foo = function(aaa,\n" +
            "  bbb,\n" +
            "  ccc) {\n" +
            "                    bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: 1, body: 10}}],
            errors: expectedErrors([[2, 2, 3, "Identifier"], [3, 2, 1, "Identifier"], [4, 2, 10, "ExpressionStatement"]])
        },
        {
            code:
            "var foo = function(aaa,\n" +
            "  bbb, ccc, ddd\n" +
            "                        eee, fff) {\n" +
            "        bar();\n" +
            "}",
            output:
            "var foo = function(aaa,\n" +
            "                   bbb, ccc, ddd,\n" +
            "                   eee, fff) {\n" +
            "    bar();\n" +
            "}",
            options: [4, {FunctionExpression: {parameters: "first", body: 1}}],
            errors: expectedErrors([[2, 19, 2, "Identifier"], [3, 19, 24, "Identifier"], [4, 4, 8, "ExpressionStatement"]])
        },
        {
            code:
            "var foo = function(\n" +
            "aaa, bbb, ccc\n" +
            "    ddd, eee) {\n" +
            "  bar();\n" +
            "}",
            output:
            "var foo = function(\n" +
            "  aaa, bbb, ccc\n" +
            "  ddd, eee) {\n" +
            "      bar();\n" +
            "}",
            options: [2, {FunctionExpression: {parameters: "first", body: 3}}],
            errors: expectedErrors([[2, 2, 0, "Identifier"], [3, 2, 4, "Identifier"], [4, 6, 2, "ExpressionStatement"]])
        }

@mysticatea
Copy link
Member Author

@not-an-aardvark Thank you for contributing!

  • 6th valid item seems to have wrong body indentation.
  • The default behavior should be current behavior (backward compatibility).
  • The last valid item, interesting. Personally, it looks good. If someone sets parameters: "first", it implies they will write the first parameter to the right of ( because parameters are aligned automatically if they wrote the first parameter to a new line. But it might imply that we should separate alignment options from indentation level options. I'd like to ask about everyone's opinion.
    1. It's OK.
    2. {parameters: 1, align: true} or something like.

@not-an-aardvark
Copy link
Member

The default behavior should be current behavior (backward compatibility).

It looks like with the current behavior, the indentation of function parameters isn't checked at all:

// This is valid

/* eslint indent: [2, 2] */
function foo(
bar,
    baz,
        qux) {
  qux();
}

So we should only check parameter indentation if the user explicitly includes the parameters option.

not-an-aardvark added a commit to not-an-aardvark/eslint that referenced this issue Sep 2, 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
No open projects
Development

No branches or pull requests

6 participants