New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checkstyle #345

Closed
wants to merge 17 commits into
base: trunk
from

Conversation

Projects
None yet
4 participants
@ham1
Copy link

ham1 commented Nov 29, 2017

Description

  • Updated to latest checkstyle (v8.5)
  • Added many more rules to checkstyle
  • Included checking of test files and more file types

Motivation and Context

Automatically enforcing some of the coding standards/conventions helps improve readability and prevents commits like "Tab police" or commits which only change formatting thus cluttering the history (or leaving it in an inconsistent state to micro-annoy subsequent readers).

How Has This Been Tested?

ant clean checkstyle -Djava.awt.headless=true -Dskip.bug52310=true -Dskip.test_https=true test

Screenshots (if appropriate):

Types of changes

  • Dev enhancement

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #345 into trunk will decrease coverage by <.01%.
The diff coverage is 57.98%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk     #345      +/-   ##
============================================
- Coverage     58.35%   58.34%   -0.01%     
- Complexity    10204    10215      +11     
============================================
  Files          1159     1159              
  Lines         74384    74344      -40     
  Branches       7361     7339      -22     
============================================
- Hits          43405    43375      -30     
+ Misses        28483    28474       -9     
+ Partials       2496     2495       -1
Impacted Files Coverage Δ Complexity Δ
...c/functions/org/apache/jmeter/functions/XPath.java 73.07% <ø> (ø) 6 <0> (ø) ⬇️
...er/protocol/http/util/accesslog/SessionFilter.java 70.9% <ø> (ø) 11 <0> (ø) ⬇️
.../jmeter/protocol/http/control/KerberosManager.java 15.21% <ø> (ø) 3 <0> (ø) ⬇️
...org/apache/jmeter/visualizers/RenderInBrowser.java 6.55% <ø> (ø) 2 <0> (ø) ⬇️
...che/jmeter/protocol/http/proxy/HttpRequestHdr.java 67.03% <ø> (ø) 45 <0> (ø) ⬇️
...ons/org/apache/jmeter/functions/Jexl2Function.java 81.13% <ø> (ø) 10 <0> (ø) ⬇️
...jmeter/protocol/http/util/HTTPResultConverter.java 43.75% <ø> (ø) 6 <0> (ø) ⬇️
.../org/apache/jmeter/config/gui/SimpleConfigGui.java 66.66% <ø> (ø) 13 <0> (ø) ⬇️
...rphan/org/apache/jorphan/gui/ObjectTableModel.java 61.01% <ø> (ø) 25 <0> (ø) ⬇️
...ctions/org/apache/jmeter/functions/JavaScript.java 85.29% <ø> (ø) 14 <0> (ø) ⬇️
... and 122 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7440523...0910c07. Read the comment docs.

@romani

This comment has been minimized.

Copy link

romani commented Nov 29, 2017

@ham1 , ReturnCount check is commented out. Please point me to place where you configure it.
If that is what you try to add, please merge this PR, create new PR with change to ReturnCount that will fail in your CI, and I will review why it why happen one day.

@ham1

This comment has been minimized.

Copy link

ham1 commented Nov 30, 2017

@romani it's commented out to stop this PR failing but where you see it commented out was where I had:

<module name="ReturnCount">
  <property name="max" value="10"/>
</module>
@romani

This comment has been minimized.

Copy link

romani commented Nov 30, 2017

Looks like a reason was in maxForVoid with default value., Right ?

@ham1

This comment has been minimized.

Copy link

ham1 commented Nov 30, 2017

Yes, that's correct, I some how missed it in the docs.

I've suggested on checkstyle/checkstyle#5305 that the error message is changed to something like:
"Return count is 2 (maxForVoid allowed is 1). [ReturnCount10]"
or
"Void return count is 2 (max allowed is 1). [ReturnCount10]"
to perhaps help prevent the confusion.

Thanks for your time in looking into an RTFM issue.

@romani

This comment has been minimized.

Copy link

romani commented Nov 30, 2017

@ham1 , if you enforce(ERROR level) more than half of Checks on this project, we can use jmeter project in each commit (in checkstyle) regression testing.

@ham1 ham1 force-pushed the ham1:checkstyle branch from abe8be8 to d10b61f Nov 30, 2017

@ham1

This comment has been minimized.

Copy link

ham1 commented Dec 1, 2017

@romani, sorry I'm not clear on what, if anything, you'd want me to do/change?

@ham1 ham1 force-pushed the ham1:checkstyle branch 3 times, most recently from ccf18d7 to 7d83fe1 Dec 1, 2017

@ham1 ham1 force-pushed the ham1:checkstyle branch from 7d83fe1 to cd58beb Dec 3, 2017

@romani

This comment has been minimized.

Copy link

romani commented Dec 5, 2017

if master branch of this project (jmeter) start to use about 80 Checks in its config with ERROR severity and no violations reported, we can start to use jmeter in out CI for regression testing.

just keep enforcing more and more rules, and pin me then you are done.

@pmouawad

This comment has been minimized.

Copy link
Contributor

pmouawad commented Dec 6, 2017

Hi team, anybody wants to merge this one ? if not I’ll do it this week-end. Thanks

@asfgit asfgit closed this in 455da75 Dec 6, 2017

@ham1 ham1 deleted the ham1:checkstyle branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment