Js-beautify: daisy-chain indentation leads to over-indentation

[rasenplanscher]

Using this via https://github.com/enginespot/js-beautify-sublime

Expected:

.foo()
.bar();

Actual:

  .foo()
  .bar();

To illustrate the problem: the current indenting can lead to EOFs like this:

      });
    });
})();

That looks like an error to me, and will prompt me to look for the cause – or worse, make me blind to the real ones =(

[evocateur]

Could you give us the full input and expected output, along with your config? I'm having difficulty correlating your expected fragment (which looks incorrect, honestly) to the closing blocks. Thanks!

[rasenplanscher]

As many things in this area, this is of course a matter of personal preference and habit rather than an objective truth.

After processing, you may see something like this:

(function () {
    'use strict';

    angular
        .module('module', [])
        .directive('appVersion', ['version',
            function (version) {
                return function (scope, elm, attrs) {
                    elm.text(version);
                };
            }
        ])
        .directive('foo', [

            function () {
                return {};
            }
        ])
        .directive('bar', [

            function () {
                return {};
            }
        ]);
})();

I would like this:

(function () {
    'use strict';

    angular
    .module('module', [])
    .directive('appVersion', ['version',
        function (version) {
            return function (scope, elm, attrs) {
                elm.text(version);
            };
        }
    ])
    .directive('foo', [
        function () {
            return {};
        }
    ])
    .directive('bar', [
        function () {
            return {};
        }
    ]);
})();

This is my configuration:

{
    "indent_level": 0,
    "indent_with_tabs": true,
    "preserve_newlines": true,
    "max_preserve_newlines": 5,
    "jslint_happy": true,
    "brace_style": "collapse",
    "keep_array_indentation": false,
    "keep_function_indentation": false,
    "space_before_conditional": true,
    "break_chained_methods": true,
    "eval_code": false,
    "unescape_strings": false,
    "wrap_line_length": 0,

    // jsbeautify options
    "format_on_save": true,
    "use_original_indentation": true
}

[Volox]

+1

[bitwiseman]

The issue you're talking about is this one indent:

    angular
        .module('module', [])

I'm going to take a minute to feel a touch of awesome that we're so close to what you want to see. In times past, we wouldn't have gotten anywhere close. :smile:

The indenting there is meant to keep clarity what elements are part of a particular statement. In the general case, the indenting is basically correct. In this specific case, you have is one very long statement, but per #200 the beautifier doesn't know that that are no significant statements after that one. The beautifier is not meant to be a fully configurable formatter - it is meant to address the general case

To add some depth to this discussion, please take a look at these examples and tell me what the formatting should look like.

alpha
    .cooker(function() {
        some
            .thing()
            .should()
            .happen();
        elsewhere
            .some_other_thing()
            .should()
            .happen();
    })
    .thenclose()
beta(zeta);
omega
    .cage(function() {
        random
            .things()
            .should()
            .happen();
        elsewhere
            .some_other_thing()
            .should()
            .happen();
    })
    .thendie()

[rasenplanscher]

I'm going to take a minute to feel a touch of awesome that we're so close to what you want to see.

Absolutely! =)

In regard to indentation, I think your example should look like this:

alpha
.cooker(function() {
    some
        .thing()
        .should()
        .happen();
    elsewhere
        .some_other_thing()
        .should()
        .happen();
})
.thenclose()
beta(zeta);
omega
.cage(function() {
    random
        .things()
        .should()
        .happen();
    elsewhere
        .some_other_thing()
        .should()
        .happen();
})
.thendie()

The maxim here is the same as for curly braces: start and end should be at the same indentation. Additionally, Douglas Crockford's code conventions prescribe the switch the way they do precisely for avoiding overindentation.

[bitwiseman]

Except that js-beautify doesn't follow crockford by default, and if you run the above through jslint, it will complain that .cooker( is at the wrong indentation.

In your example, it seems to me like it is far too easy for beta(zeta); to get overlooked.
Also, you show some daisy-chains indenting and some not. What logic should the beautifier use to decide which ones indent and which ones don't?

I'm going to leave this open - the example you provide appears to be AngularJS-based, so this idiom may gain wider acceptance over time. But it is not something we'll be able to incorporate any time soon.

[rasenplanscher]

I'm terribly sorry: I botched the indentation. None of them were supposed to be indented. And for beta(zeta); not to get overlooked, I'd use empty lines, like so:

alpha
.cooker(function() {
    some
    .thing()
    .should()
    .happen();

    elsewhere
    .some_other_thing()
    .should()
    .happen();
})
.thenclose();

beta(zeta);

omega
.cage(function() {
    random
    .things()
    .should()
    .happen();

    elsewhere
    .some_other_thing()
    .should()
    .happen();
})
.thendie();

As I said at the outset, I think it's a matter of personal taste. And specifically with single-line chains I am less inclined to reduce the indentation. But I find the multi-line case very bad and having mixed styles would be horrible, so I would definitely stick to the strategy of less indentation, always.

[bitwiseman]

You might look at #485. With this up coming fix the following will now remain unchanged when it goes through the beautifier:

(function () {
    'use strict';

    angular
        .module('module', [])
        .directive('appVersion', ['version', function (version) {
            return function (scope, elm, attrs) {
                elm.text(version);
            };
        }])
        .directive('foo', [function () {
            return {};
        }])
        .directive('bar', [function () {
            return {};
        }]);
})();

Still not what you'd like, but the beautifier will no longer force the function declaration to a newline (while still respecting a newline if you include it).

[arthur5005]

I have to +1 this request, this is especially handy when working with Promises.

Promise.resolve()
.then(function() {
  return foo.bar()
})
.then(function() {
  return foo.baz();
})
.then(function() {
 //...
}) //...
//...

This chaining can continue for a while, especially when writing a more involved api end point, and by the time you're looking at the bottom you're constantly thrown off guard with how the indentation finishes relative to something close by.

I believe it's important that all closing indentation be one level of depth difference from the next nearest.

[brandondrew]

:+1:

I believe it's important that all closing indentation be one level of depth difference from the next nearest.

To me this is the clincher that proves that extra indentation is misleading and ultimately a mistake. I realize some may feel that there is some sense in which

Promise.resolve()
  .then(function() {
    return foo.bar()
  })

seems to reflect that the then() in some sense has a parent-child relationship with Promise.resolve(), but if that is true, then so does each subsequent then() have that relationship with the previous then(). Of course there really is _no_ such parent-child relationship, and indenting as if there were all the way down would make a grand mess, so no one does it. But indenting the first then() just makes a _small_ mess instead of a huge one—it's just that some people seem willing to put up with that small mess, while some of us prefer to not have _any_ messes in our code if we can help it.

[brandondrew]

It may be nice to have the visual indication that indentation provides, but in this case it is overloading the meaning of indentation—not just indicating a new scope, but also indicating a chained method. However we already have the . to indicate a chained method, and because the . is at the beginning of the text on the line, it really provides all the (pseudo-)indentation that is needed as long as you are paying attention to it.

So it's not _really_ _just_ a matter of personal preference—it's a matter of benefits & drawbacks of both approaches. (Personal preference _is_ of course involved, because some may not care about certain drawbacks or about certain benefits, but the discussion can be more fruitful if we discuss what those benefits and drawbacks _are_, rather than just saying "I prefer _x_" or "I prefer _y_".)

I think the case is strong that the drawbacks of extra indentation are significant, while the benefits can be had another way.

Drawbacks of extra indentation for chained methods:

• your indentation is no longer a reliable indicator of scope
• your closing punctuation is likely to make you think you've made a mistake

Benefits:

• you get a larger visual indication of method chaining than just a . by itself provides (BUT you _do_ get that indication with just a .)

[Santinell]

+1

[klarkc]

+1 This is leading to Expected exactly one space between '{a}' and '{b}' errors in jslint.

Example:

gulp.task('changelog', function () {
    return gulp.src('CHANGELOG.md', {
            buffer: false
        })
        .pipe(conventionalChangelog({
            preset: 'angular' // Or to any other commit message convention you use.
        }))
        .pipe(gulp.dest('./'));
});

Errors:

4   Expected 'buffer' at column 9, not column 13.    buffer: false
5   Expected '}' at column 5, not column 9.  })

Correct way (for jslint):

gulp.task('changelog', function () {
    return gulp.src('CHANGELOG.md', {
        buffer: false
    })
        .pipe(conventionalChangelog({
            preset: 'angular' // Or to any other commit message convention you use.
        }))
        .pipe(gulp.dest('./'));
});

[ilovett]

I would very much prefer to have this as an option, primarily to avoid unnecessary extra indentation in promise chaining:

  // fetch `config.json` and then set as constant
  $http.get('config.json').then(function(response) {
    angular.module('myApp').constant('CONFIG', response.data);
  })
  .then(function() {
    angular.bootstrap(document, ['myApp']);
  })
  .catch(function(err) {
    var message = (err && err.data || err && err.message);
    console.error('Unable to bootstrap application.', err);
    window.alert('Unable to bootstrap application.' + (message ? '\n\n' + message : ''));
  });

I think the extra indentation is the equivalent of:

try {
    // 4 spaces
  } // 2
  catch () {
    // 4
  }

It makes sense if the the child token which wraps around a block of indentation is on a new line from the initial variable:

    .module('module', function() {
      // .module starts on new line, so this block has 2 indents
    })

vs.

  angular.module('module', function() {
    // .module is on the same line as the initial variable angular, so this block has 1 indent
  })

But if it can all fit on one line then the double indentation shouldnt happen.

(As it stands, the above would be linted/expected as:)

angular.module('module', function() {
    // double indent
  });

[bitwiseman]

I've marked this as an enhancement.
I'm not arguing against it being a good idea, I just don't have time.

All you "+1"-ers and those who have commented, feel free to contribute a pull request.

[wonderbeyond]

+1 want

[aecepoglu]

I've opened a PR for this

https://github.com/beautify-web/js-beautify/pull/927

if the continuous integration would finally update the PR status, it should be ready to merge.

[pietrovismara]

+1 This is boring as hell