Edgewall Software
Modify

Opened 6 years ago

Last modified 2 years ago

#10802 new task

Cleanup Javascript code

Reported by: Peter Suter Owned by:
Priority: normal Milestone: next-dev-1.3.x
Component: general Version: 1.0-stable
Severity: normal Keywords: javascript
Cc: leho@…
Release Notes:
API 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 6 years ago.
Attempt to cleanup one file as an example
t10802-clang-format-r14576.diff (122.0 KB ) - added by Christian Boos 2 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 2 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 2 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 2 years ago.
the original workflow_graph.js at r11199, after a reformat by clang-format

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by Peter Suter

Attachment: workflow_graph.js.patch added

Attempt to cleanup one file as an example

comment:1 Changed 6 years ago by 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 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 Changed 6 years ago by lkraav <leho@…>

Cc: leho@… added

comment:3 Changed 6 years ago by Christian Boos

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

I'm with you on this one ;-)

comment:4 Changed 6 years ago by Christian Boos

Keywords: javascript added

comment:5 Changed 3 years ago by lkraav <leho@…>

Cc: leho@… removed

comment:6 Changed 3 years ago by lkraav <leho@…>

Cc: leho@… added

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

comment:7 Changed 2 years ago by 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.

Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:8 Changed 2 years ago by Christian Boos

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.

Changed 2 years ago by Christian Boos

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

Changed 2 years ago by Christian Boos

Attachment: workflow_graph-psuter.js added

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

Changed 2 years ago by Christian Boos

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

Changed 2 years ago by Christian Boos

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

comment:9 Changed 2 years ago by Ryan J Ollos

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

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.
The owner will be changed from (none) to anonymous.

Add Comment


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