Coding guidelines #57

Open
olaf-k opened this Issue Jan 30, 2014 · 18 comments

Comments

Projects
None yet
8 participants
@olaf-k

olaf-k commented Jan 30, 2014

This issue has been created after a post from @dpreussner on the at.com forum.

Since the code base is still at an early stage, we should take the opportunity to lay out a set of basic rules to follow so that code style stay consistent everywhere.

These rules could be listed in the (to be created) Contributing.md file.

@PK1A

This comment has been minimized.

Show comment
Hide comment
@PK1A

PK1A Feb 28, 2014

Contributor

So, after working a very tiny bit on the #space code-base I also start to feel a need to formalize coding styles. The practical drive here is to minimize size of diffs from pull requests. If everyone uses different editor settings the code is going to be constantly re-formatted back and forth by different people.

Now, IMO, all those standardization efforts can succeed only if those are enforced automatically as part of the build. Non-conforming code should simply break a build.

Keeping the above in mind I would propose to introduce https://github.com/mdevils/node-jscs checks which is like jshint pushed a bit further. If we consider going down this path we should do it rather sooner than later as a bigger code-base will make it harder / impossible. BTW, those checks can be introduced on the folder-per-folder basis so there is a way of progressively introducing things.

WDYT?

Contributor

PK1A commented Feb 28, 2014

So, after working a very tiny bit on the #space code-base I also start to feel a need to formalize coding styles. The practical drive here is to minimize size of diffs from pull requests. If everyone uses different editor settings the code is going to be constantly re-formatted back and forth by different people.

Now, IMO, all those standardization efforts can succeed only if those are enforced automatically as part of the build. Non-conforming code should simply break a build.

Keeping the above in mind I would propose to introduce https://github.com/mdevils/node-jscs checks which is like jshint pushed a bit further. If we consider going down this path we should do it rather sooner than later as a bigger code-base will make it harder / impossible. BTW, those checks can be introduced on the folder-per-folder basis so there is a way of progressively introducing things.

WDYT?

@divdavem

This comment has been minimized.

Show comment
Hide comment
@divdavem

divdavem Feb 28, 2014

Member

For code formatting, we could use grunt-jsbeautifier to format js code, as it is done in noder-js, see here.
It has 2 modes: one to format files (to be used before committing changes to GitHub), and one to check that files are correctly formatted (to be used in travis-ci) so that we don't forget to format files.

Member

divdavem commented Feb 28, 2014

For code formatting, we could use grunt-jsbeautifier to format js code, as it is done in noder-js, see here.
It has 2 modes: one to format files (to be used before committing changes to GitHub), and one to check that files are correctly formatted (to be used in travis-ci) so that we don't forget to format files.

@marclaval

This comment has been minimized.

Show comment
Hide comment
@marclaval

marclaval Feb 28, 2014

Member

Definitively needed!

Member

marclaval commented Feb 28, 2014

Definitively needed!

@dpreussner

This comment has been minimized.

Show comment
Hide comment
@dpreussner

dpreussner Feb 28, 2014

We´ll as I brought up the issue I +1 these efforts. But I would also suggest that a general style guide is used/defined which everyone adheres to while writing the code. This minimizes the effort to "fix"/normalize it afterwards. Most of it is just a matter of personal discipline, which of course can and should be aided by tools.

But in general it would be better not to have the different styles in the beginning. To cite your aria templates contribution guidelines:

Good code is
readable
maintainable
reusable
testable
performant
documented

We´ll as I brought up the issue I +1 these efforts. But I would also suggest that a general style guide is used/defined which everyone adheres to while writing the code. This minimizes the effort to "fix"/normalize it afterwards. Most of it is just a matter of personal discipline, which of course can and should be aided by tools.

But in general it would be better not to have the different styles in the beginning. To cite your aria templates contribution guidelines:

Good code is
readable
maintainable
reusable
testable
performant
documented

@jakub-g

This comment has been minimized.

Show comment
Hide comment
@jakub-g

jakub-g Feb 28, 2014

Collaborator

Talking about the code style: If the rules are imposed by the automatic tools (jshint, style checkers/formatters), you'll learn the rules pretty quickly.

In my experience, it's waaay better to have a build-time rejections than any written guidelines because people don't read and/or forget the latter.

One little remark regarding grunt-jsbeautifier
IMO it should be forked and changed to add empty newlines at the end of the files (which is a Unix convention) in order to be better compatible with Unix tools like vim which append the newline automatically, and for git not to display the ugly "no newline at the end of file" in diffs

Collaborator

jakub-g commented Feb 28, 2014

Talking about the code style: If the rules are imposed by the automatic tools (jshint, style checkers/formatters), you'll learn the rules pretty quickly.

In my experience, it's waaay better to have a build-time rejections than any written guidelines because people don't read and/or forget the latter.

One little remark regarding grunt-jsbeautifier
IMO it should be forked and changed to add empty newlines at the end of the files (which is a Unix convention) in order to be better compatible with Unix tools like vim which append the newline automatically, and for git not to display the ugly "no newline at the end of file" in diffs

@dpreussner

This comment has been minimized.

Show comment
Hide comment
@dpreussner

dpreussner Mar 3, 2014

How about a pull request first - before forking?

How about a pull request first - before forking?

@ymeine

This comment has been minimized.

Show comment
Hide comment
@ymeine

ymeine Mar 3, 2014

Member

Personally I prefer having a formal reference for coding style. Something easier to understand than a configuration file of a (often not exhaustive) formatting tool.

Member

ymeine commented Mar 3, 2014

Personally I prefer having a formal reference for coding style. Something easier to understand than a configuration file of a (often not exhaustive) formatting tool.

@PK1A

This comment has been minimized.

Show comment
Hide comment
@PK1A

PK1A Mar 3, 2014

Contributor

@ymeine IMHO is something is not enforced automatically it will be neglected and not followed. Having formal / written guidelines are good (and I'm sure we can generate human readable description from a config file, if needed!) but if violations don't break the build we are going to catch some of those violations during code review and some others will be left unnoticed.

I really would like to automate all the things we can - I simply don't want to think about those rules while coding / doing code reviews.

Contributor

PK1A commented Mar 3, 2014

@ymeine IMHO is something is not enforced automatically it will be neglected and not followed. Having formal / written guidelines are good (and I'm sure we can generate human readable description from a config file, if needed!) but if violations don't break the build we are going to catch some of those violations during code review and some others will be left unnoticed.

I really would like to automate all the things we can - I simply don't want to think about those rules while coding / doing code reviews.

@jakub-g

This comment has been minimized.

Show comment
Hide comment
@jakub-g

jakub-g Mar 3, 2014

Collaborator

On attester, we have a grunt task (not automatic, manual so far) that formats the code and launches jshint [1]. To get some insight about how this works, you can

  1. Do some development, check that your tests passes etc. then git commit;
  2. Launch this task from command line (grunt dev)
  3. git diff to see what the formatter has changed in your code (usually it will be things like adding/removing space in certain places, like after semicolons in object literals etc.)

It's as simple as that and you don't need a verbose document ;) Do this a few times and you'll learn the rules without reading anything.

IMO what we should rather discuss is what to choose :)

  • jscs seems promising, though it's just a checker, not a formatter -> you'll have to "fix" the things by hand
  • grunt-jsbeautifier is a bit configurable, but not extremely configurable
  • spket [2] is highly configurable though it's a java app and it's not free (we have a license, but I guess we can't use it on Travis - so far we are just using it for AT locally on our dev machines)
  • esformatter [3] looks promising but it's probably not yet production ready (i haven't checked)

[1] https://github.com/attester/attester/blob/master/Gruntfile.js#L55
[2] ariatemplates/ariatemplates#991 (comment)
[3] https://github.com/millermedeiros/esformatter

Collaborator

jakub-g commented Mar 3, 2014

On attester, we have a grunt task (not automatic, manual so far) that formats the code and launches jshint [1]. To get some insight about how this works, you can

  1. Do some development, check that your tests passes etc. then git commit;
  2. Launch this task from command line (grunt dev)
  3. git diff to see what the formatter has changed in your code (usually it will be things like adding/removing space in certain places, like after semicolons in object literals etc.)

It's as simple as that and you don't need a verbose document ;) Do this a few times and you'll learn the rules without reading anything.

IMO what we should rather discuss is what to choose :)

  • jscs seems promising, though it's just a checker, not a formatter -> you'll have to "fix" the things by hand
  • grunt-jsbeautifier is a bit configurable, but not extremely configurable
  • spket [2] is highly configurable though it's a java app and it's not free (we have a license, but I guess we can't use it on Travis - so far we are just using it for AT locally on our dev machines)
  • esformatter [3] looks promising but it's probably not yet production ready (i haven't checked)

[1] https://github.com/attester/attester/blob/master/Gruntfile.js#L55
[2] ariatemplates/ariatemplates#991 (comment)
[3] https://github.com/millermedeiros/esformatter

@ymeine

This comment has been minimized.

Show comment
Hide comment
@ymeine

ymeine Mar 3, 2014

Member

@PK1A Sorry, I forgot to precise that I prefer having that too! 😄

Of course I'm in favor of automation, and +1 for generating something from a config file! However I think that a classical documentation format for coding style (in addition to the rest) allows talking about traits that go beyond what is covered by the tools we could use. Coding is not just a matter of syntax and indentation.

Well, personally I follow guidelines if I know there are some and where to find them... but I can't say it would be the case of everyone 😛

Member

ymeine commented Mar 3, 2014

@PK1A Sorry, I forgot to precise that I prefer having that too! 😄

Of course I'm in favor of automation, and +1 for generating something from a config file! However I think that a classical documentation format for coding style (in addition to the rest) allows talking about traits that go beyond what is covered by the tools we could use. Coding is not just a matter of syntax and indentation.

Well, personally I follow guidelines if I know there are some and where to find them... but I can't say it would be the case of everyone 😛

@PK1A

This comment has been minimized.

Show comment
Hide comment
@PK1A

PK1A Mar 3, 2014

Contributor

@ymeine cool, looks like we are on the same page then 👍

Contributor

PK1A commented Mar 3, 2014

@ymeine cool, looks like we are on the same page then 👍

@dpreussner

This comment has been minimized.

Show comment
Hide comment
@dpreussner

dpreussner Mar 6, 2014

So what is the status on this matter now?

So what is the status on this matter now?

@olaf-k

This comment has been minimized.

Show comment
Hide comment
@olaf-k

olaf-k Mar 6, 2014

From what I gather we'll:

  1. come up with an automatic code "linter" (to run as a pre-commit hook)
  2. document its rules in a human-readable file.
    I think @PK1A is currently checking out the first point?

olaf-k commented Mar 6, 2014

From what I gather we'll:

  1. come up with an automatic code "linter" (to run as a pre-commit hook)
  2. document its rules in a human-readable file.
    I think @PK1A is currently checking out the first point?
@benouat

This comment has been minimized.

Show comment
Hide comment
@benouat

benouat Mar 6, 2014

Member

In terms of written coding guidelines, I found these ones to be really good to me
https://github.com/airbnb/javascript

Have a look at it, it is huge, but every single usecase is illustrated with a example of bad/good snippet

Member

benouat commented Mar 6, 2014

In terms of written coding guidelines, I found these ones to be really good to me
https://github.com/airbnb/javascript

Have a look at it, it is huge, but every single usecase is illustrated with a example of bad/good snippet

@PK1A

This comment has been minimized.

Show comment
Hide comment
@PK1A

PK1A Mar 7, 2014

Contributor

@dpreussner yes, I'm starting experimenting with https://github.com/mdevils/node-jscs

Contributor

PK1A commented Mar 7, 2014

@dpreussner yes, I'm starting experimenting with https://github.com/mdevils/node-jscs

@PK1A

This comment has been minimized.

Show comment
Hide comment
@jakub-g

This comment has been minimized.

Show comment
Hide comment
@jakub-g

jakub-g Jul 15, 2014

Collaborator

BTW apart from JSCS we can also add an .editorconfig file.

http://editorconfig.org/ is a project that aims to provide a simple configuration file which will be then read by a plugin for you IDE, which in turn transparently enforces the tabs vs spaces, trailing whitespace and all those kinds of conventions. It seems there are plugins for everything except Eclipse :)

Collaborator

jakub-g commented Jul 15, 2014

BTW apart from JSCS we can also add an .editorconfig file.

http://editorconfig.org/ is a project that aims to provide a simple configuration file which will be then read by a plugin for you IDE, which in turn transparently enforces the tabs vs spaces, trailing whitespace and all those kinds of conventions. It seems there are plugins for everything except Eclipse :)

@dpreussner

This comment has been minimized.

Show comment
Hide comment
@dpreussner

dpreussner Jul 15, 2014

@jakub-g Sounds like a neat idea.

What is the general status regarding the coding conventions? I think the project is past the "adjust manually" phase...

@jakub-g Sounds like a neat idea.

What is the general status regarding the coding conventions? I think the project is past the "adjust manually" phase...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment