Code Formatting/Analysis on All Tech@NYU Projects

[ethanresnick]

Hey guys,

We've talked a few times about implementing a coding styleguide, but I don't think we ever decided on a plan of action. There are a few things to do in this regard:

1. Create/decide on a code formatting guide. I think we started this discussion, but didn't ever record our decision. Let's do that here. I, of course, like the style I've been using on the json-api repo, which you can see in action here.

2. Pick a linter, transpiler, and complexity analysis tool. Let me offer some context here. I think there are really two, completely-distinct things that tools like jshint and eslint (stupidly) bundle into one. The first is that they catch likely bugs by finding constructs that don't make much semantic sense (like using a variable before you've defined it, which likely indicates a typo). The second is that they can enforce a particular format, like which line a brace should go on.

   If enforcing a specific format really matters—and I'm a dubious that it does—then this is something that would ideally be enforced and implemented automatically, e.g. by a githook (or similar) that reformats all commits without any programmer intervention.

   The much more valuable role these tools can play, imo, is the bug prevention part (which can't be automated). And, for that, we need to assess: are eslint and jshint actually different in what types of dubious constructs they can catch? which is more powerful?

   For the sake of argument, let's assume they're about equal when it comes to catching bugs. The next thing to consider is how well they'll integrate with our other tools. In particular, there are two types of other tools I think we should be using:

   1. An ESNext to ES5 transpiler like Babel. Once you taste ES6 you really won't want to go back to the ES5 dark days.
   2. A code analysis tool like Code Climate, that can check for overly complex functions and the like.

   But here's the problem: Babel integrates well with eslint but not with jshint, while Code Climate plays nice with jshint but gives inaccurate grades when used with Babel. A bunch of people have asked for CodeClimate to support eslint, so hopefully that'll happen soon. But until it does, it seems like we have to choose between either:

   1. ES6-ified Babel code with eslint, but no Code Climate complexity analysis.
   2. ES5 code without babel, but with Code Climate complexity analysis.

   It's possible that there's a "best of both worlds" option—i.e. a complexity analysis tool other than Code Climate that plays well with Babel/eslint, or a transpiler other than Babel that plays well with Code Climate—but I haven't heard of it.

   So, if it is really between those two options, I'm much more inclined to go with number one. The productivity boost gained by switching to ES6 (and the recruiting/commitment-building advantage of having infrastructure work with the latest technologies) far outweigh the value of the complexity analysis imo. So that's what I'd advocate for if we can't find a perfect solution.

Thoughts?

[maxdumas]

I'm going to look into this more thoroughly too, particularly because I want to look into a solid toolchain for minifying and packaging the intranet as part of establishing a good module system. (Right now we're still just referencing every javascript file in index.html, which hasn't gotten too terrible yet, but it's only a matter of time.)

Regarding style guide, yes please! I've been using JSCS for my side projects which has been working well for me, but of course if we can enforce it with total impunity via a git hook then I am absolutely for that. As to what style we go with, I've generally been pretty happy with Google's style guide (adapted to use tabs instead of spaces, though :) ).

Regarding linter, I think eslint seems like it will become the more popular choice and its extensibility have me leaning towards it.

Finally, while I appreciate the features that ES6 offers, my personal gut reaction is not to use it. I don't think it gives us a huge recruiting or commitment advantage; in fact I would argue the opposite, that it creates a more esoteric toolchain where people have to learn and understand more in order to begin contributing to our software. People know ES5 and everything works with ES5, and I am perfectly happy with my productivity in ES5, especially in more structured environments like Node and Angular. I really prefer this to constantly being worried if we'll be getting everything lined up through our transpiler, whether or not we're getting accurate code metrics, and having our deployed code not be one-to-one with our actual development code. I know you're looking to the future, and ES6 is of course very different from something like LiveScript because one day everyone will be using ES6, but that's still a ways ahead. I don't think now is the time to adopt it, particularly for front-end projects.

Anyway like I said I'll try to look into this more before we meet on Wednesday. I'll try to have some more concrete research done by then.

[ethanresnick]

I won't be able to talk about this at after hours tonight (I can only drop by for a bit), so I just wanted to offer some thoughts here.

1. Let's use JSCS. It looks awesome! Maybe you guys can set up a configuration for it tonight, and we can work on getting it into our deployment process next. Using the Google Styleguide with tabs instead of spaces works for me too, with the one exception that I like to have a blank line between my conditionals, i.e.:

   if (false) {
   
   }
   
   else {
   
   }

   rather than

   if (false) {
   
   } else {
   
   }

   I find that this makes it much easier too pick up where the else branch starts. Otherwise, that can be quite easy to miss, especially as the code in the branches gets longer!

2. About ESlint. I'm in favor of it too, but it still doesn't work with Code Climate. So, even if we stick with ES5, we have to stick with JSHint too if we want the complexity analysis.
3. About ES6. I think all of @maxdumas's arguments are valid, and I'm convinced that we shouldn't go all in on ES6 just yet. That said, I do want to introduce ES6 over time, because it really is way nicer. So my thought is: what if we restrict ourselves only to the subset of ES6 supported natively on all our target platforms. That way, we wouldn't need to use a transpiler, which seems like your biggest concern, and our team could learn ES6 gradually, as browsers etc. implement it.

Thoughts?

[AbhiAgarwal]

Hey guys!

I definitely agree on all of these points. Sorry I've been a little sloppy on the code in the last semester! I notice it as I was fixing up the codebase tonight. I'll definitely keep this in mind :) This semester was just a little hectic and I know I didn't write the best code!

• I managed to implement a lot of Max's JSCS and JSHint files tonight. I did it for all the services, and external projects (all projects apart from Intranet and API). I also managed to go through the projects using Sublime and fix up the code base depending on them.

There's a couple tweaks that we need to make on them. Some things are just a little annoying, but I'll try keep them in mind and discuss them in the next meeting.

Also: I feel like we need a simple way to keep .jshintrc and .jscs updated across our codebases. Any recommendations on this front? For the current set of files we have the else has to be on the same line as the curly bracket after the if block. If we would want to change this then we'd have to copy paste them again. Could this just be doing a curl and getting the latest file (maybe from the Intranet repo)?

• I think Max and I worked out on using JSHint and that's what I formatted the codebase against. I really like the idea of using Code Climate.
• I'd really like to move over some small projects onto ES6. I looked at it tonight, and I wrote my first project with it. It really is wonderful. I'm going to try and use ES6 for a couple services, and at some point I'd love if you could help me validate my code.

I think we can start getting people more comfortable with ES6 using services. Most of them are inherently smaller codebases so people are able to learn from reading and building things on top of them.

[maxdumas]

I can agree with that. I'm okay with smaller services (like single-file services) being more experimental. It gives us a good place to sandbox new tech and they're small enough that someone trying to figure out whatever weird stuff it's using won't be too big a task.

[maxdumas]

Are we ok with where we are at on this? I am still leaning towards eslint, but it looks like we have gotten everything compliant with JSHint and JSCS. Do we still want to use eslint?

[ethanresnick]

Let's discuss at After hours today! I'm fine with jshint + jscs, because eslint isn't compatible with code climate, and that seems to be a deal breaker. But I do want to talk a bit more in person about exactly what the strategy should be for gradually introducing ES6. Like I said, you've convinced me that adding es6 comes with risks, so we'll need to decide specific rules for it for each (class of) project, and I think that's something we could do tonight.

[ethanresnick]

@maxdumas Per our discussion last...here's the compatibility table for what ES6 is already supported where. (Also, remember, the ES6 draft is finalized and has been submitted to the ECMA committee for approval.)

To make that table concrete, here's some ES6 code that should run natively in FF and Chrome, even going back 2 versions on each.

function x() { 
  "use strict";

  // let is block scoped; Set() removes duplicate values; const is constant.
  let membersSet = new Set(["Abhi", "Max", "Abhi", "Ethan"]);
  const clubName = "Tech@NYU";

  // for of iterates over array/iterator values, not keys.
  for(var member of membersSet.values()) {
   // string interpolation
   console.log(`My name is ${member}`)
  }

  // a generator function!
  function* boardMembers() {
    yield "Max"; yield "Ethan"; yield "Abhi";
  }

  // boardMembers returns an interator for for-of.
  for(var member of boardMembers()) {
    console.log(`My name is ${member}`);

    // String.prototype.includes! So much more
    // intuitive than .indexOf("") !== -1
    if(member.includes("a")) {
      console.log("I’m not Abhi!");
    }
  }
}
x();

[maxdumas]

For what it's worth, that code doesn't work in the latest Firefox. :P Firefox doesn't support includes yet.

But yeah either way let's experiment with it on our smaller projects. Especially for backend stuff I see no reason not to.