Opened 6 years ago

Last modified 3 years ago

#16506 new defect

Library files are not validating against util/Checkstyle. 14301 Errors.

Reported by: gaurav21r Owned by:
Priority: undecided Milestone: 1.15
Component: CheckStyle Version: 1.8.2
Keywords: Cc:
Blocked By: Blocking:

Description

Running checkstyle.bat (without arguments) at util/Checkstyle produces:

Found 14301 checkstyle errors. Open the file checkstyleReport.html to view the results.
Build time: 59.706 seconds

checkstyle is, I feel, a very important utility in the toolkit. Its a key differentiator against other libraries like jQuery extJS etc.

The fix is also quite easy as the tool auto fixes the files. We don't have to worry about regression as these are errors related majorly to whitespace.

Any user would want their entire project to be validated for style and not just the my/app.

I request someone from the team to please give some feedback regarding this. I can help!

Change History (10)

comment:1 Changed 6 years ago by bill

Hi gaurav21r, it would definitely be nice to cleanup the style in those files. If you want to help you could fill out the CLA and then submit patch files against the latest code (i.e. trunk, not 1.8).

comment:2 Changed 6 years ago by gaurav21r

Thanks @bill. Will do that. I must warn you it'll be a first patch of mine. Relative inexperience in contributing to Dojo source here!

Its basically all the files in dojo/ dijit/ and dojox/ ! Its somewhat of a global concern for the library.

Just wanted to know from the core committers why checkStyle isn't used to validate these files? Is checkStyle not updated regularly?

According to you, why hasn't it been done (must be reason since checkStyle has been built for this reason in the first place!) And, Will running checkStyle and fixing the source files have major regression issues?

comment:3 Changed 6 years ago by bill

I'm guessing that most people don't know about the tool, or don't run it because they already [think they] know the style guidelines of dojo.

It's also true that checkStyle hasn't been updated for a long time.

comment:4 Changed 6 years ago by gaurav21r

Whats the resolution then? Should we start submitting patches for every file or should we wait for checkStyle to be updated? Is there any road map for checkstyle?

Actually, the internal documentation of checkStyle is quite poor! While its pretty tough for a novice, it should be easy for anyone familiar with the source to update the code according to http://dojotoolkit.org/community/styleGuide

comment:5 Changed 6 years ago by bill

There are no plans to update checkstyle. I thought it was working fairly well as is, but I'm not sure.

If you want you can submit patches, but don't make a separate patch for each file. One patch can contain many files.

comment:6 Changed 3 years ago by dylan

Component: GeneralCheckStyle

comment:7 Changed 3 years ago by bill

Probably we should use jscs rather than checkstyle. It does the same thing, and it's not abandoned. There are still lots of files that need to be updated for style; in particular all of the Intern test files have too many spaces.

comment:8 Changed 3 years ago by dylan

Fair enough... I think the Intern tests were written following the Dojo 2 style guidelines instead of the Dojo 1 style guidelines. At this point, I've somewhat given up, because I think our original style guidelines don't match what most of us wish they were today.

comment:9 Changed 3 years ago by dylan

Milestone: tbd1.12

Will leave open for anyone that wants to create pull requests against code style standards.

comment:10 Changed 3 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.