Cleanup Javascript code

[Peter Suter]

Once we have Javascript minification (#10672) in place, we could:

• Switch from two spaces indentation to four spaces.
• Add copyright headers.
• Add documentation and other comments.
• Perhaps take the opportunity to cleanup code in general.

TracDev/CodingStyle#JavaScript advises to follow Douglas Crockford's style (with minor modifications). Perhaps using his ​JSLint might help doing this more consistently.

(See #633, #5997 for CSS cleanup.)

[Peter Suter]

Attempt to cleanup one file as an example

[Peter Suter]

As an example I attempted to clean up workflow_graph.js. It includes a copyright header, documentation comments and is JSLint clean with the default options (including four spaces indentation). If that style seems too different we could experiment with enabling or disabling some ​options, or try ​JSHint.

File	Original size	Minified size
workflow_graph.js (trunk@11199)​	8'527	5'732
workflow_graph.js (with attachment:workflow_graph.js.patch​)	13'553	5'597

(Minified with the YUI compressor.)

[Christian Boos]

I'm with you on this one ;-)

[lkraav <leho@…>]

wtf, i was already subscribed 3 years go. lol, talk about automated clicking.

[Christian Boos]

Besides the manual steps you described like adding a copyright header, we should consider using a tool for reformatting. I recently used clang-format for that, works pretty well.

That is… if we keep using JavaScript, as I'd really favor a shift to CoffeeScript which is more fun to code in, with an indentation-based syntax not unlike that other language we use.

[Christian Boos]

So here's what ​clang-format could do for us: t10802-clang-format-r14576.diff​.

In this example, I used LLVM-3.9.0-r258122 and the following .clang-format file:

---
Language: JavaScript
BasedOnStyle: Google
IndentWidth: 2
AlignConsecutiveAssignments: true
AlignConsecutiveDeclarations: true
AlignOperands: true
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
...

An indent of 2 is what we currently use. I also tried 4, but if we limit ourselves to short line width (which is good), we quickly end up with populating only the right half of the screen… So I think keeping and indent of 2 is preferable.

Command-line used at the root of the checkout, with the above .clang-format file also present at that level:

clang-format.exe -i $(find trac -name \*.js -not -name jquery\* -not -name excanvas.js)

Also, here's how it looks like on a full file (the specific example chosen by Peter):

• trunk/trac/htdocs/js/workflow_graph.js@11199​
• workflow_graph-clang-format.js​, the above, reformatted by clang-format
• workflow_graph-psuter.js​, the manual reformatting (+ doc) done by Peter in comment:1
• workflow_graph-psuter-clang-format.js​, the above, reformatted by clang-format

If everyone thinks it would be a good new style, then I could do the reformatting on both the 1.0-stable branch and trunk (in order to facilitate later merging), and add a jsindent target to the Makefile, so that it's easy to re-run it before committing new changes to the .js files.

One issue though is that clang-format 3.9 is not yet officially released and I haven't checked yet the older versions to see if the JavaScript support is as good. It's important that everyone would use the same version, as the output will be different with different versions.

[Christian Boos]

the diff on current trunk, after having run clang-format 3.9 on our .js files

[Christian Boos]

the full text of the cleaned-up .js, from comment:1

[Christian Boos]

the full text of the cleaned-up .js, from comment:1, after a reformat by clang-format

[Christian Boos]

the original workflow_graph.js at r11199, after a reformat by clang-format

[Ryan J Ollos]

Milestone renamed

[Ryan J Ollos]

Milestone renamed