search

impress / impress.js

It's a presentation framework based on the power of CSS3 transforms and transitions in modern browsers and inspired by the idea behind prezi.com.
http://impress.js.org
MIT License
37.66k stars 6.66k forks source link

Code style decision #529

Closed FagnerMartinsBrack closed 8 years ago

FagnerMartinsBrack commented 8 years ago

In this issue we should discuss how we are going to handle code style.

This is not for a discussion of "spaces vs tabs" or "semicolon or not", this assumes that the value on code style lies in the consistency, so there are some alternatives here:

  1. Pick whatever standard is most used in the codebase right now and stick to it, later using an automated tool like JSCS if we want to automate it (for now we try to fix in code review until we automate it). If a standard cannot be infered by the impress.js file or index.html file, then we can just choose one.
  2. Use a code style that already exists (airbnb, standard, semistandard, jquery core style, etc.)

Proposal 1 will take more time, but the team will be more comfortable. Proposal 2 prevents unnecessary bikeshedding, but the team might be a little uncomfortable.

So, should we use proposal 1 or proposal 2. Do you have another proposal?

@impress/mergers

FagnerMartinsBrack commented 8 years ago

I will wait until 14/03/2016 (AEST), if no comment is made by then I will make decision so this doesn't keep hanging forever.

bartaz commented 8 years ago

For sure the biggest value of code style is in consistency. Rest is details and preference.

Code style I used in original impress.js code is quite dated, somehow based on jQuery (at least how it looked then) and it already evolved for me and I would do it differently now.

The biggest change to do (already mentioned in #453) is use of whitespace. At time of writing impress.js I thought it would be cool to use whitespace to keep indentation even in empty lines. It looked nice when having visible whitespace in editor. But it's quite painful to maintain, especially that even git itself complains about it and most of editors have an option to automatically remove trailing white space. And I do it now. So for sure I would vote for no trailing white space and change that in current code base.

Apart from that I don't have much preference. Especially that it's quite likely I won't be the one writing code. Still it should be full proof (end lines with semicolons, etc) and readable. It would be helpful to have some automatic check and just use a code style that is default or easy to configure in some tool. I was just using jshint (which doesn't do much code style checks).

So I guess I would be more towards 2nd option - to choose something existing and adopt it, rather than having discussions on our own style and need to document it.

FagnerMartinsBrack commented 8 years ago

Maybe the best way would be to use a standard that has the potential to change the least amount of code in the project. Since @bartaz stated that he kind of already started with jquery styleguide, what about enforcing it? Anyone else have better suggestions?

https://contribute.jquery.org/style-guide/js/

We can even copy the jscs preset and .jshintrc from them. This style even removes the white-space identation in which @bartaz already expressed the intent to change.

bcipolli commented 8 years ago

jquery styleguide, what about enforcing it? ... We can even copy the jscs preset and .jshintrc from them.

That sounds right. If I understand properly, that means we can set up continuous integration (CI) with Travis (or others), so that styles can be enforced with minimal effort from us (maintainers).

FagnerMartinsBrack commented 8 years ago

jQuery it is then.

FagnerMartinsBrack commented 8 years ago

PR #535.