FinalLocalVariable does not check for loop variable

[Ruslan Diachenko]

This topic is related to the issue: https://github.com/checkstyle/checkstyle/issues/20

Assignee:  Bhavik Patel

[Ruslan Diachenko]

Hi Bhavik Patel,

According to your pull request: https://github.com/checkstyle/checkstyle/pull/630

1. Please explain the changes in .classpath, what for?
2. Next time you make a pulll request please provide some description about what you have done
3. Don't use such commit messages:

#20

They say nothing useful

4. Your changes decreased the coverage, please fix
>> Coverage decreased (-0.02%)

5. I don't see any changes which are related to the actual fix
6. Before making a pull request please squash all your commits into one

[Bhavik Patel]

Hi Ruslan Diachenko,

sorry for not describing in detail.

>> .classpth changes : actually i did not change anything in .classpath manually. it was done by eclipse automatically. i thought it is necessary for project to be built.

>> https://github.com/Bhavik3/checkstyle/blob/889c7bcfe036c46ad40904ce8469939c84cd69cd/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputFinalLocalVariable.java#L134 should be declared final.

    FinalLocalVariableCheck.java is not checking for this variable (because it is not checking for FOR_EACH_CLAUSE variable). following is the lone of code:

    https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java#L112

    so, I remove this line. as there is not any conflict with removing this line.
 
    now there are many places where FOR_EACH_CLAUSE variable is not declared final. so i changed it.

>> may be I am doing something wrong. point me out where I am going wrong. so that i can fix it and improve it.

Thanks,
Bhavik.

[Ruslan Diachenko]

Hi,

Ok, seems fine. Could you please:

1. Rollback changes in .classpath. There shouldn't be changes in this file
2. Squash your commits into one and provide a link to your commit

[Bhavik Patel]

OK

[Bhavik Patel]

Hi Ruslan,

following is the commit in my fork repository. I built it on my local system and all fine. you look at it:

https://github.com/Bhavik3/checkstyle/commit/92f392623197355319de1b6c8cc8b5d532b424da

I will do PR after your suggestion.

[Ruslan Diachenko]

Hi,

Please do pull request and include the issue number.

[Ruslan Diachenko]

>> Coverage should not be decreased. Please update unit tests as well

You can use code coverage tools (e.g.: Cobertura, Eclemma, etc.). Most of them can be installed as plugins to Eclipse.

Than you run FinalLocalVariableCheckTest within a code coverage tool and analyze the source of FinalLocalVariableCheck. All uncovered code branches will be highlighted. So you can update FinalLocalVariableCheckTest to cover missed cases.

[Bhavik Patel]

 

 showing 100% coverage on my local system for FinalLocalVariableCheckTest

[Ruslan Diachenko]

See the attached file - the result of running FinalLocalVariableCheckTest with Eclemma. Not 100% but 99.4% and there're some code branches which can be covered :)

[Bhavik Patel]

ya, I seen it after posting.

[Ruslan Diachenko]

Hi,

According to you pull request. Seems you have problems with squashing you commits as there are changes which are not related to your issue.

[Ruslan Diachenko]

Please follow these instruction about the development process: https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Development-workflow-with-Git%3A-Fork%2C-Branching%2C-Commits%2C-and-Pull-Request where "sevntu-checkstyle" can be replaced to "checkstyle".

The goal: we need only your changes in one commit message.

[Bhavik Patel]

Hi,

I noticed that. but actually, there are other developer's commits in between my two commits. so when I squashed them, their commits also squashed into me.

[Bhavik Patel]

 

 It is good idea to make separate branch. so that no other commits can come in between my commits and I can squash them. At the end, I will merge my squashed commit to my origin/master and finally push into upstream.