Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Styling #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Styling #3

wants to merge 2 commits into from

Conversation

joskuijpers
Copy link
Contributor

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
Copy link
Owner

brianc commented Sep 7, 2014

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. 😄

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
Copy link
Contributor Author

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
Copy link
Owner

brianc commented Nov 20, 2014

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants