millermedeiros / esformatter

ECMAScript code beautifier/formatter
MIT License
970 stars 91 forks source link

Wrong indentation using 0.4.0 #193

Closed peol closed 10 years ago

peol commented 10 years ago

I'm trying to update @jzaefferer 's grunt-esformatter task to latest esformatter and stumbled upon issues with the indentation behaviour.

Using default settings (snippets from unit tests on grunt-esformatter):

Source

module.exports = function(grunt) {

  grunt.registerMultiTask('esformatter', 'Format JS files', function() {
    var options = this.options({});
    this.files.forEach(function(f) {
      f.src.filter(function(file) {
        return grunt.file.exists(file);
      }).forEach(function(file) {
        var content = grunt.file.read(file);
        grunt.file.write(file, esformatter.format(content, options));
      });
    });
  });

};

Formatted

module.exports = function(grunt) {

  grunt.registerMultiTask('esformatter', 'Format JS files', function() {
    var options = this.options({});
      this.files.forEach(function(f) {
        f.src.filter(function(file) {
          return grunt.file.exists(file);
        }).forEach(function(file) {
          var content = grunt.file.read(file);
          grunt.file.write(file, esformatter.format(content, options));
        });
      });
    });

  };

this.files.forEach(function(f) { gets the wrong indentation and the snippet is wrongly indented after that.

peol commented 10 years ago

The line var options = this.options({}); seems to be the culprit, removing that fixes the indent issue. I haven't been able to debug it to see why though.

millermedeiros commented 10 years ago

It's probably the MemberExpression.getIndentEdges (inside hooks folder). MemberExpression indentation was all wrong before 0.4 and that was the last thing I fixed. But I did not try it out..

one way to debug it is to run with DEBUG=esformatter.indent ./bin/esformatter your_test_file.js

peol commented 10 years ago

Thanks, trying to debug but I'm new to both esformatter and rocambole/AST so I'm not sure what I'm looking at :) I got a failing test-case added here: https://github.com/peol/esformatter/commit/ce5484dba865d6300e1bee6f15253ffc2c4370a7

And when running DEBUG=esformatter.indent ./bin/esformatter test/compare/default/indentation-out.js I get:

  esformatter:indent [getIndentLevel] type: ThisExpression, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: Identifier, value: undefined +3ms
  esformatter:indent [getIndentLevel] type: MemberExpression, value: 1 +0ms
  esformatter:indent [transformNode]: hook returned no edges +0ms
  esformatter:indent [getIndentLevel] type: ObjectExpression, value: 1 +1ms
  esformatter:indent [transformNode] type: ObjectExpression, edges: {, 
 +0ms
  esformatter:indent [indentInBetween] originalStart: {, start: }, end: 
, level: 1 +0ms
  esformatter:indent [getIndentLevel] type: Identifier, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: CallExpression, value: 1 +0ms
  esformatter:indent [transformNode]: hook returned no edges +0ms
  esformatter:indent [getIndentLevel] type: VariableDeclarator, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: VariableDeclaration, value: undefined +0ms
  esformatter:indent [specialNodeType] indent: 1 +0ms
  esformatter:indent [transformNode]: hook returned no edges +0ms
  esformatter:indent [getIndentLevel] type: BlockStatement, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: Identifier, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: FunctionExpression, value: 1 +0ms
  esformatter:indent [transformNode] type: FunctionExpression, edges: {, 
 +0ms
  esformatter:indent [indentInBetween] originalStart: {, start: 
, end: 
, level: 1 +1ms
  esformatter:indent [getIndentLevel] type: AssignmentExpression, value: 1 +0ms
  esformatter:indent [transformNode]: hook returned no edges +0ms
  esformatter:indent [getIndentLevel] type: ExpressionStatement, value: undefined +0ms
  esformatter:indent [getIndentLevel] type: Program, value: undefined +0ms
foo = function() {
  var bar = this.baz({});
  };
millermedeiros commented 10 years ago

by looking at this log I think the problem is on the ObjectExpression edges.. see that indent start is the } and not { and it ends on a line break (which I'm assuming is not the correct line break - since indent goes till end of file).

millermedeiros commented 10 years ago

ahh, just opened the ObjectExpression hook and the logic for getIndentEdges is wrong when isChainedMemberExpressionArgument(node) == true... it's getting the first line break before the end token (}), it needs to actually do _tk.findInBetweenFromEnd(node.startToken, node.endToken, _tk.isBr)

a pull request including the test case and fix would be highly appreciated!

millermedeiros commented 10 years ago

PS: if the indentEdge.endToken or indentEdge.startToken is falsy it won't indent the node, so you don't need to worry about edge cases.. the findInBetweenFromEnd should be safe in this case