Edgewall Software
Modify

Opened 12 years ago

Last modified 14 months ago

#10802 new task

Cleanup Javascript code

Reported by: Peter Suter Owned by:
Priority: normal Milestone: next-dev-1.7.x
Component: general Version: 1.0-stable
Severity: normal Keywords: javascript
Cc: leho@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.)

Attachments (5)

workflow_graph.js.patch (22.4 KB ) - added by Peter Suter 12 years ago.
Attempt to cleanup one file as an example
t10802-clang-format-r14576.diff (122.0 KB ) - added by Christian Boos 8 years ago.
the diff on current trunk, after having run clang-format 3.9 on our .js files
workflow_graph-psuter.js (13.2 KB ) - added by Christian Boos 8 years ago.
the full text of the cleaned-up .js, from comment:1
workflow_graph-psuter-clang-format.js (12.4 KB ) - added by Christian Boos 8 years ago.
the full text of the cleaned-up .js, from comment:1, after a reformat by clang-format
workflow_graph-clang-format.js (8.6 KB ) - added by Christian Boos 8 years ago.
the original workflow_graph.js at r11199, after a reformat by clang-format

Download all attachments as: .zip

Change History (16)

by Peter Suter, 12 years ago

Attachment: workflow_graph.js.patch added

Attempt to cleanup one file as an example

comment:1 by Peter Suter, 12 years ago

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 sizeMinified 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.)

comment:2 by lkraav <leho@…>, 12 years ago

Cc: leho@… added

comment:3 by Christian Boos, 12 years ago

Milestone: next-major-releasesnext-dev-1.1.x

I'm with you on this one ;-)

comment:4 by Christian Boos, 12 years ago

Keywords: javascript added

comment:5 by lkraav <leho@…>, 9 years ago

Cc: leho@… removed

comment:6 by lkraav <leho@…>, 9 years ago

Cc: leho@… added

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

comment:7 by Christian Boos, 8 years ago

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.

Version 0, edited 8 years ago by Christian Boos (next)

comment:8 by Christian Boos, 8 years ago

Version: 1.0-stable

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):

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.

by Christian Boos, 8 years ago

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

by Christian Boos, 8 years ago

Attachment: workflow_graph-psuter.js added

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

by Christian Boos, 8 years ago

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

by Christian Boos, 8 years ago

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

comment:9 by Ryan J Ollos, 8 years ago

Milestone: next-dev-1.1.xnext-dev-1.3.x

comment:10 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.3.xnext-dev-1.5.x

Milestone renamed

comment:11 by Ryan J Ollos, 3 years ago

Milestone: next-dev-1.5.xnext-dev-1.7.x

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.