Opened 7 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 7 years ago by
comment:2 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
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 4 years ago by
Component: | General → CheckStyle |
---|
comment:7 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
Milestone: | tbd → 1.12 |
---|
Will leave open for anyone that wants to create pull requests against code style standards.
comment:10 Changed 3 years ago by
Milestone: | 1.13 → 1.15 |
---|
Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.
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).