Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18793 closed enhancement (implemented)

add checkstyle task

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.0.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: ctip
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Add checkstyle task based on Google Java Style Guide and adopt this to the Metrics specific rules as detailed in #18733.

Child Tickets

Attachments (2)

0001-Extended-.gitignore-replaced-ERNIE-by-CollecTor-and-.patch (10.0 KB) - added by iwakeh 3 years ago.
0002-Added-checkstyle-task.patch (12.7 KB) - added by iwakeh 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by iwakeh

This is available checkstyle (5.9-1) on jessie.

But, preferably some version >= 6.0 should be used (see checkstyle release notes) and 6.17 is available.

Last edited 3 years ago by iwakeh (previous) (diff)

comment:2 Changed 3 years ago by karsten

Well, using the version in Debian stable would make this a lot easier, in particular if we ask contributors to check the style of their patches before submitting them (which I'd want to do). What particular features or fixes makes using version 6.0+ preferable?

comment:3 Changed 3 years ago by iwakeh

If we aim at using most of Google Style, this note from release 6.0:

Google style is now covered to maximum of Checkstyle ability.

I'll take a look at how much change is actually necessary for using the provided Google Style xml from the Checkstyle project with version 5.9. or checkstyle.

comment:4 Changed 3 years ago by karsten

Ah, that's indeed an important requirement then. Thanks for looking whether we can do this with 5.9. Let me know if we need to adapt an existing style, and I'll help with that. I think I'd also be fine having 90% of things checked automatically, that's 90% more than today. Thanks!

comment:5 Changed 3 years ago by iwakeh

Status: newneeds_review

Please find attached two patches:

The first just fixes ERNIE to CollecTor naming in comments and logging statements and adds entries to gitignore.

The second adds the checkstyle task (ant checks) to build.xml
I used the latest checkstyle-all-6.17 jar from checkstyle.sf.net; it contains all dependencies for checkstyle.

The configuration is the taken from checkstyle's google-checks.
I put it in resources/metrics-checks.xml.

The results of running ant checks are written to generated/checkstyle_report.txt.

Changed 3 years ago by iwakeh

comment:6 Changed 3 years ago by karsten

Looks good! Merged and pushed to master. Thanks!

I noticed a possible problem with the .gitignore file though, and it's not one that you're introducing but one that we should fix soon: we're not prefixing any directories or files with a slash. So, if we ever add a package *.lib.*, we'll ignore that in Git because it contains a lib directory in its path. See also:

http://stackoverflow.com/questions/24139478/when-to-use-leading-slash-in-gitignore

Please find my branch task-18793 with a trivial patch on top of yours (current master).

But, how do we continue with pleasing checkstyle? Do you want to write patches, or should I start with the easy issues which produce 90% of warnings, or how do we do this?

comment:7 Changed 3 years ago by iwakeh

Good to use more specific .gitignore patterns!
All looks fine!

Regarding the style corrections:
It would be great if you could make the bulk corrections!
I though about tackling style issues when touching the code anyway;
like with the additional renaming patch in this issue.

I'll add a coding style polishing issue with a "nice to have soon list".
(code style polishing ticket: #18931)

Anyway, for not having used any coding metrics before in this project and
also considering that many checkstyle complaints are due to the old style,
this code base is in pretty good shape checkstyle-wise.

Last edited 3 years ago by iwakeh (previous) (diff)

comment:8 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

comment:9 Changed 3 years ago by karsten

Great, pushed that last commit to master. I'll start working on #18931 soon. Feel free to close this ticket unless anything remains to be done. Thanks!

comment:10 Changed 3 years ago by iwakeh

Resolution: implemented
Status: merge_readyclosed

Thanks!
All done, closing.

comment:11 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.0.0

Added to milestone for first release.

Note: See TracTickets for help on using tickets.