search

brianc / node-libpq

Simple, low level native bindings to PostgreSQL's libpq from node.js
112 stars 42 forks source link

Styling #3

Open joskuijpers opened 10 years ago

joskuijpers commented 10 years ago

The first commit only adds semicolons and stuff. In the second commit, I've run JS Beautifier (tabsize 2) over all the JS code. If you don't like its result (consistent use of spaces, mostly, after keywords), tell me, and I will produce a patch without it.

The annoying this of using 2 spaces instead of 4 spaces, is that Beautifier puts 1 tab (=2/4 spaces) in front of the require block elements. It lines up awesomely when using 4 spaces:

var x = require('x'),
    y = require('y');

I do realize you might have your own config of a beautifier, or you TOTALLY hate this. Then I am sorry and I will remove it :P I just see this style a lot in Node.JS.

brianc commented 10 years ago

Haha - I don't TOTALLY hate this and definitely appreciate the thought behind it. The semi-colons stuff is definitely groovy. I switch back and forth between using semi-colons or not, but as my team at work has decided to use semi-colons all the way I figured I should start doing it in open source too for consistency and practice. I missed a few places though here & am thankful you caught 'em. :smile:

As for the spaces between function () and if ()...while I don't hate it, I don't code that way and already kept a consistent style throughout the entire code base with keeping the parenthesis up right against the statement. That's a decade old habit I definitely don't see any reason to change now, so I don't think the beautification results for changing that is something we should add. Otherwise as soon as I start writing code again I'm gonna make the same mistake, then have another round of commits to run the beautifier, etc over and over. If there's a way to change the beautifier to not put those spaces in I'd happily integrate it into the project along with an npm package.json script or something else to make it automatically run on commits or when the tests run or something. This is how it's done in node-postgres w/ jshint.

joskuijpers commented 10 years ago

Haha that is just fine. I also wanted no spaces there but I could not configure it to remove spacing between anonymous functions and its brackets. :/ So I went with this style. Can you just take the patch for the first commit and close this?

brianc commented 9 years ago

I think imma close this one because it no longer cleanly merges, and I tried to go manually address all the semi-colon issues - you cool with that?