Skip to content
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

Pull #565: added new check MoveVariableInsideIfCheck #565

Merged
merged 2 commits into from May 25, 2017

Conversation

@rnveach
Copy link
Contributor

@rnveach rnveach commented May 9, 2017

Adding check MoveVariableInsideIfCheck.
Also added cases that can cause false positives.

Will rename commit and provide regression.

Fixes in Checkstyle: checkstyle/checkstyle#4328
There was a small bug in my original implementation, so there may be more violations in Checkstyle than before.

@rnveach rnveach force-pushed the rnveach:move_variables_inside_if branch from 8472442 to 3bb9f76 May 9, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 9, 2017
@rnveach rnveach force-pushed the rnveach:move_variables_inside_if branch from 3bb9f76 to 9bc223f May 9, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 9, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 9, 2017
@rnveach rnveach force-pushed the rnveach:move_variables_inside_if branch from 9bc223f to 8ffba84 May 9, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 9, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 9, 2017
@rnveach rnveach requested a review from romani May 9, 2017
@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 9, 2017

Fixed violations in sevntu.
I changed the violation slightly so it points to the variable instead of the block so the suppression will be next to the variable, which makes more sense.

Travis failure is because of changes needed in Checkstyle.

@rnveach rnveach changed the title minor: added new check MoveVariableInsideIfCheck Pull #565: added new check MoveVariableInsideIfCheck May 9, 2017
@romani
Copy link
Member

@romani romani commented May 9, 2017

CI has to be green.
Can you send to checkstyle repo with fixes?
Javadoc build item has to fixed too

@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 9, 2017

Regression failed with an problem in utility, made a issue for it. Will fix it.
Will make updates to checkstyle. Will have to add suppressions in it for a check that doesn't exist yet.

@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 10, 2017

Here is the regression report: http://rveach.no-ip.org/checkstyle/regression/reports/30/

Unfortunately there is another bug with the newly introduced patch only report that I did not see when making the change. I will fix that too.
The reports are still readable, it is just too many files with no violations are listed.

@romani
Copy link
Member

@romani romani commented May 10, 2017

@rnveach , why reports are so weird and list file with no violations ?

@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 10, 2017

@romani This is the bug I mentioned and the fix is in checkstyle/contribution#228
I was unaware that the differentiating compare in patch-diff-report-tool was dropping these files and not the xml reader when it found nothing.
Since this is a patch only run, it literally added everything in the patch report.

@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 11, 2017

@romani CI passed.

Copy link
Member

@romani romani left a comment

some items:

@@ -165,6 +165,8 @@
<regex><pattern>.*.checks.coding.EitherLogOrThrowCheck</pattern><branchRate>88</branchRate><lineRate>99</lineRate></regex>
<regex><pattern>.*.checks.coding.ForbidThrowAnonymousExceptionsCheck</pattern><branchRate>81</branchRate><lineRate>97</lineRate></regex>
<regex><pattern>.*.checks.coding.MapIterationInForEachLoopCheck</pattern><branchRate>90</branchRate><lineRate>98</lineRate></regex>
<regex><pattern>.*.checks.coding.MoveVariableInsideIfCheck</pattern><branchRate>98</branchRate><lineRate>100</lineRate></regex>
<regex><pattern>.*.checks.coding.MoveVariableInsideIfCheck.Holder</pattern><branchRate>89</branchRate><lineRate>100</lineRate></regex>

This comment has been minimized.

@romani

romani May 12, 2017
Member

can we make coverage 100% ? to completely remove this lines

This comment has been minimized.

@rnveach

rnveach May 12, 2017
Author Contributor

I removed the code for 100%. Regression will tell us if there are any NPEs.

@@ -212,6 +212,8 @@
<property name="ignoreMethod" value="false"/>
</module>

<module name="com.github.sevntu.checkstyle.checks.coding.MoveVariableInsideIf" />

This comment has been minimized.

@romani

romani May 12, 2017
Member

please move it to coding group , and fix indentation

This comment has been minimized.

@rnveach

rnveach May 12, 2017
Author Contributor

Fixed. Sorry for this.

/**
* <p>
* Checks if a variable is only used inside if statements and asks for it's
* declaration to be moved there too.

This comment has been minimized.

@romani

romani May 12, 2017
Member

we do not have xdoc.
So javadoc is all that you can show to user for details. http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/index.html
Please update it to have wide description and examples.

This comment has been minimized.

@rnveach

rnveach May 12, 2017
Author Contributor

Fixed.

DetailAST ifNode = ifNodeGiven;

// -@cs[SingleBreakOrContinue] Too complex to break apart
while (true) {

This comment has been minimized.

@romani

romani May 12, 2017
Member

while TRUE and suppression .... I do not like this.
I can not propose smth better now, but it smells.

This comment has been minimized.

@rnveach

rnveach May 12, 2017
Author Contributor

loop is going through all ifs of if-elseif chain.
while loop could be changed to make sure if node is an if node.
Is this change ok?

However, the suppression is for the number of breaks and this change won't help that.
I think it will hurt code readability by changing it, but thats just my opinion.

This comment has been minimized.

@romani

romani May 25, 2017
Member

moved to #587

@rnveach rnveach force-pushed the rnveach:move_variables_inside_if branch from 8ffba84 to ccb78b9 May 12, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 12, 2017
rnveach added a commit to rnveach/sevntu.checkstyle that referenced this pull request May 12, 2017
@rnveach rnveach force-pushed the rnveach:move_variables_inside_if branch from ccb78b9 to f393975 May 12, 2017
@coveralls
Copy link

@coveralls coveralls commented May 12, 2017

Coverage Status

Coverage increased (+0.02%) to 98.708% when pulling f393975 on rnveach:move_variables_inside_if into 7feb71c on sevntu-checkstyle:master.

@rnveach
Copy link
Contributor Author

@rnveach rnveach commented May 12, 2017

New regression: http://rveach.no-ip.org/checkstyle/regression/reports/40/

No changes or NPEs.

@romani
romani approved these changes May 25, 2017
@romani romani merged commit 4b3838e into sevntu-checkstyle:master May 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@romani romani added the approved label May 25, 2017
@romani romani added this to the 1.24.0 milestone May 25, 2017
@romani
Copy link
Member

@romani romani commented May 25, 2017

now we can do 1.24.0 release

@rnveach rnveach deleted the rnveach:move_variables_inside_if branch May 25, 2017
kariem added a commit to kariem/sevntu.checkstyle that referenced this pull request Jul 26, 2018
kariem added a commit to kariem/sevntu.checkstyle that referenced this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants