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

Create style guide #19

Closed
cletusc opened this issue Nov 7, 2013 · 21 comments
Closed

Create style guide #19

cletusc opened this issue Nov 7, 2013 · 21 comments
Labels
feature Something we don't already have implemented to the best of knowledge but would like to see. team biz This is similar to a meta discussion.
Milestone

Comments

@cletusc
Copy link
Contributor

cletusc commented Nov 7, 2013

We need consistent formatting of code and we should define it very explicitly. We can always do cleanup commits, or even have someone specifically assigned to do code cleanup.

@jerone
Copy link
Contributor

jerone commented Nov 7, 2013

We can look at existing style guides, like the one from jQuery: http://contribute.jquery.org/style-guide/

@simonzack
Copy link
Member

Something I'm not quite sure about is the use of camel case vs underscore, which I think carries over from java. Jquery has it in it's guidelines, but it's imo much harder to read. Which casing should we adopt?

@cletusc
Copy link
Contributor Author

cletusc commented Nov 7, 2013

camel case vs underscore

IMO, camel case. Thinking in the future here, if we use underscore.js (_(stuff).func()) or some sort of i18n (usually seen as __('Some string here.')), the extra underscores from variable names might confuse some.

I would recommend basing a lot of it on idiomatic.js, which is what I kind of follow already albeit with some white space changes.

@jerone
Copy link
Contributor

jerone commented Nov 7, 2013

I don't mind which style guide we choose, as long as make use of tabs (instead of spaces). With tabs I can choose my own size (2, 4, 8, ...), which is 4 for me personally.

@cletusc
Copy link
Contributor Author

cletusc commented Nov 7, 2013

make use of tabs (instead of spaces)

Agreed, I propose we use tabs for indentation, and spaces for alignment of anything else after the indentation is correct, should we need to align anything.

@simonzack
Copy link
Member

make use of tabs (instead of spaces)

Agreed, I propose we use tabs for indentation, and spaces for alignment of anything else after the indentation is correct, should we need to align anything.

Same here, I absolutely love tabs.

@sizzlemctwizzle
Copy link
Member

I despise tabs, and so does most of the open source community from my experience. Challenge: find a popular open source project that uses tabs for indentation. Also the tab width on GH is huge so our code will look like shit here. Also, this is another barrier to entry. Spaces work everywhere, but tab width must be set where ever (if even possible) you plan to view the code.

Find a proper source code editor that will add the appropriate number of spaces when you hit the tab button (I know Notepad++ supports this). If this is a must have for you guys, I will write a node script that converts tabs to spaces and vice versa.

Also, I vote for the Greasemonkey style guidelines, with a few exceptions for the parts that apply to Firefox addon development.

@joesimmons What do you think? (I hope tagging a user brings it to their attention)

@joesimmons
Copy link
Member

Can we use Douglas Crockford's code conventions?
It's idiomatic, from what I understand of that word.

I'm for using spaces. I used to use tabs but spaces are actually better.
Notepad++, as sizzle said, can replace tabs (as you type them) with a certain # of spaces.
I really like indentation to be 4 spaces, as it's clear to me when a block is indented. I could use 2, but I'd have to switch back and forth all the time when developing for the site, and developing my own scripts.

@cletusc
Copy link
Contributor Author

cletusc commented Nov 7, 2013

We could look into integrating an editor config to simplify things--there are plugins for all major text editors by the looks of it. This would fix any issues with tabs, line endings, and EOF newlines.

Most of all, I want to avoid any major debate between tabs and spaces. As we don't have a unified opinion, let's find a middle ground--editorconfig should help.

For clarification, I am OK with GM style guide aside from the space/tab issue.

@sizzlemctwizzle
Copy link
Member

Can we use Douglas Crockford's code conventions?
I agree with all that, but it's very similar to the Greasemonkey style guide.
I really like indentation to be 4 spaces, as it's clear to me when a block is indented. I could use 2, but I'd have to switch back and forth all the time when developing for the site, and developing my own scripts.
I like using 2 spaces because you indent 4 spaces for a statement that continues onto the next line. If we use 4 spaces, that means 8 spaces on a continuation and it will quickly explode.
We could look into integrating an editor config to simplify things
So we just plug this into our editor of choice and it saves the files according to the defined guidelines? That page doesn't explain it very well.

@jerone
Copy link
Contributor

jerone commented Nov 7, 2013

And so starts the big spaces versus tabs discussion :)

As it stands now, we have:

  • 3 users for tabs
  • 1 user for using 2 spaces
  • 1 user for using 4 spaces

@cletusc
Copy link
Contributor Author

cletusc commented Nov 7, 2013

So we just plug this into our editor of choice and it saves the files according to the defined guidelines?

That's the idea I think. I'll look more into it when I get home, but jQuery uses it and I'm sure many others as well. It's a good idea if it works.

And so starts the big spaces versus tabs discussion :)

What I was trying to avoid... :P

@sizzlemctwizzle
Copy link
Member

And so starts the big spaces versus tabs discussion :)

I thought this debate was settled before I was even born, but apparently I was wrong. I thought we were all spacemen by now.

That's the idea

Well if it works, then win-win.

@jerone
Copy link
Contributor

jerone commented Nov 7, 2013

Does Node.js has a style guide?

@sizzlemctwizzle
Copy link
Member

It's written in C++.

@joesimmons
Copy link
Member

I can do whatever is needed. I'd rather use spaces, though.

@jerone
Copy link
Contributor

jerone commented Nov 8, 2013

NPM has a small style guide: https://npmjs.org/doc/coding-style.html
A highly appriciated style guide has been made by Felix Geisendörfer: http://nodeguide.com/style.html
A good reason to use spaces over tabs has been posted here: http://stackoverflow.com/a/10503479/108448

@ghost ghost assigned cletusc Nov 9, 2013
cletusc added a commit that referenced this issue Nov 10, 2013
@cletusc
Copy link
Contributor Author

cletusc commented Nov 10, 2013

I've committed an initial draft of the style guide. Let's discuss it on the mailing list.

@ghost ghost assigned joesimmons and cletusc Nov 22, 2013
@joesimmons
Copy link
Member

I just pushed a style guide created primarily from Douglas Crockford's code conventions, taking pieces from Cletus' initial guide.

It's not a verbatim copy of Doug's conventions, but many of those are included, except for a few.

@sizzlemctwizzle
Copy link
Member

Well we have a style guide now, so I'm closing this issue.

sizzlemctwizzle referenced this issue Nov 27, 2013
…us and unnecessarily tedious. I'm never going to do that and I don't expect anyone else to.
@jerone
Copy link
Contributor

jerone commented Nov 30, 2013

Then we can close the branch too...

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jun 23, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jun 25, 2014
* This is considered bad form and could eventually break this controller if V8 updates to strict mode.
* Think I got all of them on a manual survey of this controller.

Applies to OpenUserJS#200 and should be clarified in OpenUserJS#19
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jun 25, 2014
* Don't add a space when adding a newline... this just makes things messy if an editor doesn't support `.editorconfig`
* Separate out Comma Separator from Comma Operator
* Remove the `var statements` from Comma Separator *(plus it's an operator btw)* because we don't currently allow that under [Variable Declarations](https://github.com/OpenUserJs/OpenUserJS.org/blob/master/STYLEGUIDE.md#variable-declarations) and can confuse contributors.

Applies to OpenUserJS#19
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jun 26, 2014
Martii pushed a commit that referenced this issue Jun 26, 2014
Some strong emphasis and additions/clarifications for #19
@Martii Martii added the feature label Jul 3, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2014
* `switch` is designed to handle multiple logic cases since ES3 and forward.
* There is no requirement for the prior restrictions in any of the styleguides currently mentioned
* Noted that curly braces can be used for code folding, otherwise they are mostly useless imho and as far as I know
* Added `expressionNth` to the sample with a simple fall through.
* GMs requirement of adding a comment for fallthroughs seems rather tautological since this is how the statement has been in the C family of languages and all of its derivatives for many decades.
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 8, 2014
…ead of curly braces.

* Apply some more of the styleguide to the STYLEGUIDE.md
* Clarify in notation that sometimes a `switch` may not always be the best option for readabilty in some cases.
* Leaving the choice of curly braces up to each contributor for the time being. Code folding is nice but ususally `switch` isn't that complex enough to need that.

Applies to OpenUserJS#245 and OpenUserJS#19
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 8, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 8, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Jul 8, 2014
* Almost verbatim copied from sizzle

Applies to OpenUserJS#245 and OpenUserJS#19 and recomendend at OpenUserJS#208 (comment)
sizzlemctwizzle added a commit that referenced this issue Jul 11, 2014
Loosen up the death grip on #19 with `switch`
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Aug 18, 2015
* Misplaced `var` declarations ... closer to current ES5.x implementation for some others just misplaced
* Reordered on first use for all vars... not as critical but good for debugging
* Changed `s3` early initialization... shouldn't need to be **and if it did** it should always be `NOTE:`d with a comment
* Some newlines for white space usage... easier reading/debugging
* Any `require` statements all at top... `require` functions become cached and always stay loaded after first use... so really shouldn't be attempting to optimize these in our code... only perceived benefit is startup pm time but is loaded elsewhere so no real benefit.
* Some notation of `WARNING:` statement on incomplete error handling
* White-space trim that my editor didn't catch on a previous commit even though it is on all the time everywhere. :\
* `WARNING:` note added for resaving script model object for no apparent reason... again should be notated.
* Max line length of 100 conformance

Applies with OpenUserJS#285, OpenUserJS#486, OpenUserJS#390, and OpenUserJS#19 ... perhaps some more... very minimal change in project behavior here which is why it is isolated.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Something we don't already have implemented to the best of knowledge but would like to see. team biz This is similar to a meta discussion.
Development

No branches or pull requests

6 participants