Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9278 closed enhancement (fixed)

Add a build tool to check for style guide violations

Reported by: Shane O'Sullivan Owned by: Shane O'Sullivan
Priority: high Milestone: 1.4
Component: BuildTools Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

The Dojo Style Guide is extremely comprehensive, and covers a wide variety of coding standards. While many of these are very difficult to enforce automatically, there are also many style guideline violations that can be detected using a build tool.

I propose adding a new build tool that generates a style guide violation report. A patch is attached which shows an initial approach that could be taken for this.

To use it, apply the patch and then, in util/buildscripts, type:

build action=checkstyle

This generates a report for all of Dojo, Dijit and Dojox, excluding tests and demos folders.

To check just a single folder, type:

build action=checkstyle checkstyleDir=dojo/base

The folder specified is relative to the root of the Dojo folder structure.

Once the report is generated, open the checkstyleReport.html file.

Attachments (5)

checkstyle.2.patch (17.1 KB) - added by Shane O'Sullivan 10 years ago.
Updated patch
checkstyle.3.patch (19.0 KB) - added by Shane O'Sullivan 10 years ago.
Updated patch that makes it easier to integrate with other code. checkstyleUtil no longer opens and reads a file itself.
checkstyle.4.patch (19.9 KB) - added by Shane O'Sullivan 10 years ago.
Update patch, files moved to their own util/checkstyle folder
checkstyle.5.patch (21.4 KB) - added by Shane O'Sullivan 10 years ago.
Updated patch, support for specifying individual files, throwing exception on failure
checkstyle.patch (67.0 KB) - added by Shane O'Sullivan 10 years ago.
Improved help

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Adam Peller

If we had such a tool, it could be applied as a pre-checkin step also. We've discussed this before, but never gotten far. A simple, but controversial approach would be to just adopt Crockford's jslint rules.

comment:2 Changed 10 years ago by Adam Peller

Component: GeneralBuildTools

Changed 10 years ago by Shane O'Sullivan

Attachment: checkstyle.2.patch added

Updated patch

Changed 10 years ago by Shane O'Sullivan

Attachment: checkstyle.3.patch added

Updated patch that makes it easier to integrate with other code. checkstyleUtil no longer opens and reads a file itself.

Changed 10 years ago by Shane O'Sullivan

Attachment: checkstyle.4.patch added

Update patch, files moved to their own util/checkstyle folder

Changed 10 years ago by Shane O'Sullivan

Attachment: checkstyle.5.patch added

Updated patch, support for specifying individual files, throwing exception on failure

Changed 10 years ago by Shane O'Sullivan

Attachment: checkstyle.patch added

Improved help

comment:3 Changed 10 years ago by Shane O'Sullivan

(In [17569]) Refs #9278 Adding a build to to check for style guide violations. It provides a command line interface to run the checks on all JavaScript? files in the Dojo tree, a HTML file to view and manipulate the results, and a PHP file that can be used to save the fixed files.

The readme.txt file contains instructions on its use, and the commmand

checkstyle.sh help

prints more help text.

comment:4 Changed 10 years ago by Shane O'Sullivan

(In [17579]) Refs #9278 Added checks for characters being inside strings, and omits them from style checks. Also rearranged the UI to show the file list and edit panel in the same tab.

comment:5 Changed 10 years ago by Nathan Toone

(In [17580]) Refs #9278 - change line endings so it will run on MacOS

comment:6 Changed 10 years ago by Nathan Toone

(In [17581]) Refs #9278 - update shell script to point to the correct js file

comment:7 Changed 10 years ago by Shane O'Sullivan

(In [17582]) Refs #9278 Added code to fix spaces after if, else, while and switch statements when the Make Fixes button is clicked in the UI.

comment:8 Changed 10 years ago by Shane O'Sullivan

(In [17585]) Refs #9278 Added code to replace two consecutive spaces with a tab

comment:9 Changed 10 years ago by Eugene Lazutkin

Huh. I thought we used the "4 spaces per tab" convention. And replacement should be done when certain conditions are met, e.g., proper tab locations, not arbitrary N spaces.

comment:10 Changed 10 years ago by Shane O'Sullivan

Eugene,

More accurace fixes are of course welcome. However, I've used these on a few files and they seem to work very well. As I replace four spaces with tabs first, then two spaces, I cover both the case where people have correctly set their editor to use four spaces for tabs, and where they have incorrectly set up their editors to use two spaces per tab.

If you'd like to make some modifications, please feel free to do so.

comment:11 Changed 10 years ago by Shane O'Sullivan

(In [17586]) Refs #9278 Fixed bug in PHP file that was preventing files being saved.

comment:12 Changed 10 years ago by Shane O'Sullivan

(In [17615]) Refs #9278 Added exceptions for regular expressions. Given that almost any characters can appear in a regular expression, they should not be checked for style violations. This change essentially treats regular expressions as comments.

comment:13 Changed 10 years ago by Shane O'Sullivan

Status: newassigned

comment:14 Changed 10 years ago by Shane O'Sullivan

(In [20437]) Refs #9278 Fixes issues with saving the file when a directory is not specified

comment:15 Changed 10 years ago by Shane O'Sullivan

(In [20451]) Refs #9278 Fixes issues with loading a file for editing in the Checkstyle viewer when the user has filtered by directory.

comment:16 Changed 10 years ago by Shane O'Sullivan

Resolution: fixed
Status: assignedclosed

Checkstyle tool is now quite stable and usable. Any future issues can be logged as separate tickets, so closing this one.

comment:17 Changed 10 years ago by bill

(In [20604]) Doc permission problems on mac, refs #9278.

Note: See TracTickets for help on using tickets.