Tabs to 2 spaces, idiomatic.js / jsbeautify, cleaning

[tony]

This may be a bit bigger of a change than needed, but here is what it includes:

• Tabs to spaces
  Using a vim mode of ts=2 sts=2 et sw=2
  and a jsbeautify mode of let g:jsbeautify = {'indent_size': 2, 'indent_char': ' '}.

I was pretty careful to double check for tabs and grunt. And I did fine-tooth comb to see what the ramifications of jsbeautify did:

• added spacing to control structures
  (var i=0; i<files.length; i++) would be (var i = 0; i < files.length; i++)

  I'm agnostic to this. Idiomatic.js does add spaces in this way. https://github.com/rwldrn/idiomatic.js/#spacing. Seeing this control structure a thousand times, it's tempting to scrunch, but for someone new reading it, I'd tolerate it so other's can read.

• var declaration

  it will remove the tabs indentation in:

var moo,
     hey = 'ho';

and change to

var moo,
hey = 'ho';

idiomatic.js is ok with it. It's ok to read, and easier to maintain. All formatting on these seem ok by idiomatic, but indentation is processed differently based on if varable is set, if it's inlined, if it's a literal notation, etc. This needs to be handled outside of a script if it's to look totally consistent.

• function declarations

  Right now, in some areas we will have a space between arg parens and function function (arg) and also no space between the ending arg paren and initial bracket function (arg){.

  This changes it to:

  function(arg) {.

  Also it corrects spaces in argument lists that may have been forgotten.

  Unlike idiotmatic however, it does not add spaces in between arguments ( arg ) and ( arg1, arg2 ). I haven't seen many projects that do this, probably because it's a pain in the ass.

  • Converts tabs to spaces on CSS also.
  • Removes some final stray /* globals */ grunt comments.

Do we want to split this into smaller parts, handling just tabs->spaces, opting out of formatting, css, etc? Take a step back and try reading it anew. Imo it feels more consistent with viewing source on backbone.js.

[travisbot]

This pull request passes (merged 765867ac into d42b5fd).

[addyosmani]

Nice work, @tony. Thanks for taking the time to better enforce some of iditomatics principles to the codebase. I've made one minor comment but so far the rest looks readable. I'll spend a little more time inspecting the changes.

@dustinboston do you have any thoughts on this?

[sindresorhus]

While you're at it, I would suggest using multiple var statements, var declarations at the top of the function, space around arguments, and more spacing top/bottom where it fits (currently the code is a little cramped vertically). I would also suggest you use 'use strict'; in every file.

Example:

From:

function foo(bar,fum) {
    var ret,
        hello = 'Hello';
    for (var i = 0, l = bar.length; i < l; i++) {
        if (bar[i] === hello) {
            ret = fum(out);
        }
    }
    return ret;
}

To:

function foo( bar, fum ) {
    var i, l, ret;
    var hello = 'Hello';

    for ( i = 0, l = bar.length; i < l; i++ ) {
        if ( bar[ i ] === hello ) {
            ret = fum( out );
        }
    }

    return ret;
}

[addyosmani]

I'm more than happy to consider any suggestions @sindresorhus has to offer in this area. He's by far the biggest code style nazi I know and I mean that in the nicest way possible :)

multiple var statements, var declarations at the top of the function, space around arguments, and more spacing top/bottom where it fits

I agree with this. We've also been using use strict in most of our other projects without issue and I have no qualms with us applying this to Aura.

[dustinboston]

@addyosmani @tony looks good. I'm up for use strict. I'm not a huge fan of inner white space (around arguments), or multiple var statements, but I'm ok with it as long as we agree to keep it consistent.

[sindresorhus]

The space stuff is just for readability, but theres actually some very good arguments for the use of multiple var statements.

But yeah, consistency ++

[dustinboston]

@sindresorhus You're right. Those are good arguments. I'm sold :-)

[travisbot]

This pull request passes (merged 369e4bc7 into d42b5fd).

[travisbot]

This pull request passes (merged 74a3d27e into d42b5fd).

[travisbot]

This pull request passes (merged e5223332 into d42b5fd).

[travisbot]

This pull request passes (merged ddc89583 into d42b5fd).

[tony]

I'm going to take another look at this and var tomorrow. Until then my npm-debug.log is compromised. Better cancel debit cards as a precaution ^^.

[travisbot]

This pull request passes (merged 876b248 into d42b5fd).

[travisbot]

This pull request passes (merged 72d79da into d42b5fd).

[tony]

@sindresorhus Thanks for that link on multiple vars. I think that readability / maintainability has improved.

I've taken a look at backbone.marionette's approach to newlines. Function properties are separated with a new line. There is no beginning or serial new line inside of the marionette module unless the first property begins with a comment.

Marionette.CollectionView = Marionette.View.extend({
  constructor: function(){
    Marionette.View.prototype.constructor.apply(this, arguments);
    this.initChildViewStorage();
    this.initialEvents();
    this.onShowCallbacks = new Marionette.Callbacks();
  },

  // Configured the initial events that the collection view 
  // binds to. Override this method to prevent the initial
  // events, or to add your own initial events.
  initialEvents: function(){
    if (this.collection){
      this.bindTo(this.collection, "add", this.addChildView, this);
      this.bindTo(this.collection, "remove", this.removeItemView, this);
      this.bindTo(this.collection, "reset", this.render, this);
    }
  },

  // Handle a child item added to the collection
  addChildView: function(item, collection, options){
    this.closeEmptyView();
    var ItemView = this.getItemView();
    return this.addItemView(item, ItemView, options.index);
  },

  ...

We don't really have a standard for newlines, but for now I did a few consistency changes, most of which are for new lines:

• "use strict"; under define, with no new line
• New line before end of module
• New line after var declarations at top of functions.

Module properties new lines:

• No new line at beginning of first module property, unless there is commenting (backbone.marionette)
• No new line after last module property.

I'm inclined toward making Backbone Aura exportable to a http://jashkenas.github.com/docco/. I made a ticket #89 to look into it later.

[tony]

@sindresorhus Any best practices / habits you have for new lines? @dustinboston?

I also saw the code comment for single quote 'use strict';, I'm okay with that, even though used to double quotes in this specific instance. Anyone else?

[sindresorhus]

I've taken a look at backbone.marionette's approach to newlines. Function properties are separated with a new line. There is no beginning or serial new line inside of the marionette module unless the first property begins with a comment.

So, good and clean code style isn't always about doing the same in all instances. You have to take into consideration the context.

Just an example from the first file I could find:

function fetchModuleList( keyword, cb ) {
    var moduleList = JSON.parse( localStorage.getItem( 'npm-' + keyword ) ) || [];
    var url = 'http://isaacs.iriscouch.com/registry/_design/app/_view/byKeyword?startkey=[%22' +
        keyword + '%22]&endkey=[%22' + keyword + '%22,{}]&group_level=3&callback=?';

    $.getJSON( url ).done(function( data ) {
        var urls = $.map( data.rows, function( el ) {
            var moduleName = el.key[1];
            var moduleExists = !!$.grep( moduleList, function( el ) {
                return el.name === moduleName;
            }).length;

            if ( !moduleExists ) {
                return 'http://isaacs.iriscouch.com/registry/' + moduleName + '?callback=?';
            }
        });

        if ( urls.length ) {
            $.whenMany( $.getJSON, urls ).then(function() {
                moduleList = $.map( arguments, function( el ) {
                    return el[0];
                });

                localStorage.setItem( 'npmlist-' + keyword, JSON.stringify( moduleList ) );
                localStorage.setItem( 'npmlist-timestamp-', new Date() );

                cb( moduleList );
            });
        } else {
            cb( moduleList );
        }
    });
}

fetchModuleList( 'gruntplugin', function( modules ) {
    var template = $('#plugin-list-template').html();
    $('.main').html( _.template( template, { data: modules } ) );
});

As you can see, I usually place a newline after var, but not always. My main principle is to group logical pieces and separate them from the rest. That usually means declaration, statements, etc, have spacing between them (as seen above).

"use strict"; under define, with no new line

Yes, I do that too.

[tony]

I think we should wrap up this PR and address the larger conversation in it's own issue #92. My PR's are biting at a few too many things.

@addyosmani @dustinboston Anything else for the moment? Look OK to you?

[addyosmani]

Looks good to me!

[dustinboston]

@tony FYI, looks good.