Skip to content

Modified all files to use spaces instead of tabs.#197

Closed
jsundquist wants to merge 1 commit intoOpenF2:1.4-wipfrom
jsundquist:1.4-wip
Closed

Modified all files to use spaces instead of tabs.#197
jsundquist wants to merge 1 commit intoOpenF2:1.4-wipfrom
jsundquist:1.4-wip

Conversation

@jsundquist
Copy link

As discussed on twitter. I went through and updated all JS files to include 4 spaces instead of tabs. Also updated the html files as they were a mixture of four spaces, tabs, and 2 spaces. Lastly, updated the handlebar template files while I was at it to match everything else.

@montlebalm
Copy link
Member

We can make a decision to change F2's whitespace characters from tabs to spaces, but as it is this PR violates the current contribution guidelines.

Google recommends 2 spaces, we enforce tabs equivalent to 4 spaces.

https://github.com/OpenF2/F2/wiki/Coding-Standards

@montlebalm
Copy link
Member

Following-up after seeing the full context from Twitter. Thanks for taking the time to make the modifications and we'll consider whether it makes sense to change.

@markhealey
Copy link
Member

Thanks @jsundquist. I missed your comment on going with 4 spaces — what is the thinking around 2 spaces vs 4? I think 2 spaces is what the cool kids are doing.

@jsundquist
Copy link
Author

@markhealey I personally prefer four spaces over two spaces. I don't think code is as easy to read and follow with only two spaces. A standard tab character used to be either four or eight depending on the system you were using, I believe this is why most people default to using four characters.

@markhealey
Copy link
Member

I've reviewed this, looks good overall.

@jsundquist whatever method you used to convert to spaces did some weird things with curly braces and expanding/flattening some function definitions and statements in the sdk/src directory. Nothing major, just not tab/space related. As an example, compare old and new _isPlaceholderElement in sdk/src/container.js

@montlebalm we can update the contrib guidelines.

Any objections with merging?

@montlebalm
Copy link
Member

I imagine there are a variety of personal opinions on whitespace among the core contributors. There's no technical reason to change, so if we do it we should adopt a common style. Here's what I found googling for "javascript style guide":

I can't recommend switching F2 to 4 spaces given that it doesn't appear to be widely used in the community. Based on what I've seen, I'd argue for no change or 2 spaces.

@markhealey
Copy link
Member

@jsundquist thoughts?

@jsundquist
Copy link
Author

Coming from the php community, our standards have been set to four spaces and I believe this is because a standard tab character is four spaces. Since they have set the standard to use spaces instead of the tab character, that would be my guess as to why they went with four space.

That said, each community is different. Within this repository there is a mixture of tabs and spaces within a number of files. Also if you mix your html and javascript files they should all follow the same standard. I personally feel its easier to read indentation when you look at four characters than two characters, however I am open to either way.

The argument that I would give for four spaces over two spaces is readability. With only two spaces you may have a difficult time trying to determine where a curly brace ends a for loop vs an if block within the loop depending on how large the block is. With four spaces you can get a little bit better definition.

In some of the articles I just looked up, many people point out the main reason some of these companies went with two spaces over four spaces is so they can fit more characters within a given line if the line limit is set to 80 or 120 characters. Douglas Crockford actually suggests to use four spaces when working with JavaScript files.

@markhealey
Copy link
Member

Good discussion here, thanks. Couple comments:

  1. There's a consensus that consistency is important in the repo, and with various files in various styles, there's a bit of work to do to standardize. This PR helps with that.
  2. This PR modified 3rd party source code which we cannot and do not want to change.
  3. If the source code is going to change, we're going to go with 2 spaces. There are far more examples of modern best practices using two, plus there are benefits including the one you mentioned about line lengths.

As a next step, we're going to close this PR because of #2 and #3 above.

@jsundquist we'd be thrilled with a follow-up PR (again based on 1.4-wip) with only the following source directories & files standardized to 2 spaces:

  • docs/src
  • docs/src/template
  • docs/src/sdk-template
  • docs/bin
  • examples
  • sdk/src excepting sdk/src/third-party
  • sdk/f2.nuspec.tmpl
  • tests
  • Everything in the root excepting ./F2.latest.js and LICENSE

Any files not included above are either auto-generated in a Grunt task or are 3rd party code.

@markhealey markhealey closed this Dec 17, 2014
@jsundquist
Copy link
Author

@markhealey Sounds great. I will see if I can find time to work on it later this week or next.

@markhealey
Copy link
Member

Awesome thanks @jsundquist

@markhealey markhealey added this to the 1.5.0 milestone Dec 17, 2014
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.

3 participants