tamzinblake / js3-mode

A chimeric fork of js2-mode and js-mode
GNU General Public License v3.0
181 stars 13 forks source link

Comma first in "Class" style declarations #60

Closed mbriggs closed 12 years ago

mbriggs commented 12 years ago

Hey Thom

lets say you are using backbone, you end up with a lot of things that look like this

Foo = Backbone.Model.extend({
  method1: function(){},
  method2: function(){}
})

if you do comma first, you probably want to do something like this

Foo = Backbone.Model.extend({
  method1: function(){}
, method2: function(){}
})

or

Foo = Backbone.Model.extend(
{ method1: function(){}
, method2: function(){}
})

In the first case, JS3 will indent everything to line up with the opening paren (which is probably the correct behavior). The second case, JS3 will indent the inner object by 1, also probably the correct behavior in "normal" argument lists.

I completely understand if this is completely impossible (indent like this, except for when I don't want it, then indent like that), but I figured I would bring it up anyways.

tamzinblake commented 12 years ago

I assume the comma-last case is working as expected.

In the first case, js3-lazy-commas should indent that properly.

In the second case, yes, the inner object gets indented by 1. I'm not sure what a special case for that would look like - at a bare minimum, continued expressions are indented one step, though I believe that can be turned off globally.

"Indent like this, except for when I don't want it" is an achievable goal in cases where "when I don't want it" can be specified precisely, and I do try to implement as many of those as possible.

mbriggs commented 12 years ago

here is some cases to describe what i am talking about. The problem with defining "when I don't want it" is the most precise description I can give you is "in the case of the method extend, or other methods emulating class-like declaration"

//working fine
Foo = Backbone.View.extend({
  foo: function(){
    var a = { foo: 1
            , bar: 2
            }
  },
  bar: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
})

// my ideal
Foo = Backbone.View.extend({
  foo: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
, bar: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
})

// although, would also want to support
$.extend(Foo.prototype, new Bar(), {
  foo: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
, bar: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
})

// "broken" cases

// with js3-lazy-comma
Foo = Backbone.View.extend({
  foo: function(){
    var a = { foo: 1
  , bar: 2
            }
  }
, bar: function(){
  var a = { foo: 1
, bar: 2
          }
}
})

// without js3-lazy-comma
Foo = Backbone.View.extend({
  foo: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
                           , bar: function(){
                             var a = { foo: 1
                                     , bar: 2
                                     }
                           }
})
tamzinblake commented 12 years ago

Yeah, js3-lazy-commas assumes that you're going to use that style for all multiline comma-separated lists, and otherwise the assumption is that you're not. Really, that was a hack, and it really should just figure out which style you're using and indent appropriately - I don't think it should be very hard, now that I'm looking at it again.

tamzinblake commented 12 years ago

The latest commit should work, without js3-lazy-commas. I've only implemented it for object literals, array literals, function calls, and function argument lists, and probably only the first one is relevant.

mbriggs commented 12 years ago

it is a lot closer, but it looks like it is "outdenting" the body and closing of the last function


Foo = Backbone.View.extend({
  foo: function(){
    var a = { foo: 1
            , bar: 2
            }
  }
, bar: function(){
  var a = { foo: 1
          , bar: 2
          }
}
})
tamzinblake commented 12 years ago

You might have js3-consistent-level-indent-inner-bracket set to t - that always indents function bodies one step from the beginning of the line containing the function def, which in this case is the comma.

That one is also mostly a hack to deal with different people's styles. I should probably see about fixing the default behavior there.

mbriggs commented 12 years ago

Yeah, I have that turned on. Basically, the style i use is by the book NPM style http://www.makelinux.com/man/1/N/npm-coding-style

the reason I have consistent-level-indent-inner-bracket on is because if you do semi-colon first, this is an extremely common pattern

;(function(){
  // do something here
}())

and without that setting on, I get an extra two spaces of indentation.

Thanks for all the hard work you put into this mode, indentation is hard work and dealing with the bajillion variations people have in javascript is a pretty monumental task :)

I'll get a pull request in for that add-to-globals defun in the next day or so

tamzinblake commented 12 years ago

@mbriggs Issue #62 addresses a non-hackish solution for those cases.