Project coding conventions

[Foxandxss]

Hello, as @pkozlowski-opensource knows, I am on a journey of reading every line of code of the repo and one of the goals is to collaborate.

He said that well, the code is awesome but there are some old stuff that could be improved and that is what I am here :)

What I see so far and I only have like 15% done is that in tests (have to check the code also) there are no conventions at all.

I know that is problematic to have conventions in a project with all this contributors but that doesn't mean that we can't change that.

What I suggest is:

Discuss about what conventions should we have and then update CONTRIBUTORS.md with all the rules a contributor should follow in order to write tests.

Then update all the unit tests to match this rules.

I see all kind of stuff:

Strings, I saw all kind of combinations:

"text"

'text'

'<div id="foo">'
"<div id='foo'>"
"<div id=\"foo\">"

I suggest here that we always use single quotes for everything and in the case of html strings, use double quotes for attributes.

Variable names:

I have seen a mix of $scope and scope.

If my conventions here are not wrong, we should use scope for directives and $scope for everything else. (That could be problematic in the case we have a test that cover both directives and directives' controller.

More... Injections, somethings injections like _foo_ and others just foo.

We should use underscore only when we are going to assign that parameter to a variable that we want to have the original name. I mean, this makes no sense:

scope = _$rootScope_;

If you're not going to have a $rootScope variable, just leave out the underscores.

The biggest problem is that in one test file you can see a lot of mixed conventions.

I wouldn't mind to apply this conventions in my way of reading the code if we agree in what conventions we should follow.

[douglasduteil]

Adding stricter jshint could be a start

I actually have the same problem with all the repos of the organisation. I'm pushing forward common jshint rules .

It then can be extended as will

[chrisirhc]

I agree, this bothered me but at the time I thought we need a more coordinated effort to push for conventions.

Another thing that bothered me was the use of arrays joined (array.join) with newlines to form strings. Strings should just be concatenated with whatever character they need (space or newline). This is a styling issue and doesn't have any performance impact.

[chrisirhc]

We also need to watch out with the use of beforeEach on the root describe block, as I saw while looking into #1499 .

[Foxandxss]

Yes, there is a lot of stuff that could be improved and well, that is why we have tests, to improve what is needed.

I agree with the jshint rules, but there are stuff that jshint doesn't cover (like the angular conventions).

[pkozlowski-opensource]

OK, so, personally I'm not too crazy about those code conventions as I'm wary of all the un-productive discussions it might trigger (spaces vs. tabs etc.). But I see this issue being brought to the table from time to time, so let's make it a bit better.

My only comment here is that whatever we decide should be automatically verifiable and should fail the build when a non-compliant code is detected. Otherwise we can agree on one thing but the code can get quickly desynchronised.

In practical terms we could:

• discuss project coding conventions (we can start a gist or discuss it in this issue)
• tighten jshint / eshint rules
• use http://editorconfig.org/ to enforce consistent formatting rules

@Foxandxss @caitp would you be up to animating this discussion?

[Foxandxss]

One of my concerns was that, it can get quickly desynchronized.

I am not familiar with those kind of tools so I things like... "Don't inject stuff with underscores when you're not assigning them to a variable with the original name" I don't think this can be automatically verifiable.

So I guess we should discuss what needs to be improved and can be automatically verifiable.

Since I am copying all the tests by hand, I always spot some edges and I fix them. But we really need some conventions (I put my opinion in the first message).

[pkozlowski-opensource]

@Foxandxss I think we can do quite a number of checks with eshint but I didn't dig too much into it.

Could you try to start a separate doc (ghist, wiki page, google doc) and try to list all the conventions you would like to see in this project? Then we could discuss and see how to enforce those by using various tools.

[bekos]

What about using eslint and create custom rules?

[Foxandxss]

Well, I created a pad, everybody can collaborate: http://piratepad.net/DTPS2lGgdQ

[pkozlowski-opensource]

Awesome. So far I've got nothing against taking those as conventions.

[chrisirhc]

@bekos +1 to using eslint. I wanted to suggest that too as I've read about it and it sounds promising. Went to read the code as well and it seems quite easy to add custom rules.

[wesleycho]

Closing in favor of #4758.