ga-wdi-boston / node-template

Other
2 stars 16 forks source link

Allow spaces in method shorthand syntax #3

Closed jrhorn424 closed 8 years ago

jrhorn424 commented 8 years ago

Currently:

var y = {
-    fn () {    // BAD
+    fn() {     // GOOD
       return 42;
    }
}

I prefer the space (as does Ember), but I don't mind if it is missing. Change to .jscsrc, please add alphabetically by key:

+   "disallowSpaceAfterObjectKeys": {
+    "allExcept": ["method"] 
+  },
gaand commented 8 years ago

@jrhorn424 Thoughts on this:

const fn = function () {
    return 42;
};

let o = {
  fn,
};

versus this:

let o = {
  fn () {
    return 42;
  },
};

?

Does it depend? With closures? I don't know enough about ember to say how that influences the preferred syntax, but I think I prefer the former.

jrhorn424 commented 8 years ago

It's idiomatic in Ember to define methods inline while inheriting (extending) another object. The usual pattern looks like:

import Ember from 'ember';

export default Ember.Component.extend({
  actions: {
    close () {
      return this.sendAction('close');
    }
  }
});

So the linter change allows this sort of code. In general, as you do, I prefer defining methods separately from the export.

jrhorn424 commented 8 years ago

This also makes the linting for method shorthand consistent with linting for function definitions, which allows a space before parens.

gaand commented 8 years ago

So, do we require the space? From node-template

  "requireSpacesInAnonymousFunctionExpression": {
    "beforeOpeningRoundBrace": true,
    "beforeOpeningCurlyBrace": true
  },

I actually prefer no space, but perhaps consistency should win.

jrhorn424 commented 8 years ago

There were inconsistencies between the linter in atom and the linter on the command line. I was under the impression that the AirBnB preset used spaces, so when I declared those settings explicitly, the inconsistency in tools was addressed.

Inelegant, but works.