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

Comments

Projects
6 participants
@mysticatea
Copy link
Member

commented May 3, 2016

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();
}
@doberkofler

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

+1

@BYK

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

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

@ghost

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2016

👍 from me.

@nzakas

This comment has been minimized.

Copy link
Member

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.

@robations

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2016

@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

This comment has been minimized.

Copy link

commented Jul 8, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2016

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

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

Working on this.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

@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

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

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

@gyandeeps gyandeeps closed this in f4fcd1e Sep 6, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.