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

FOR_EACH_CLAUSE variable is not declared final #648

Merged
merged 1 commit into from Mar 6, 2015

Conversation

Bhavik3
Copy link
Contributor

@Bhavik3 Bhavik3 commented Feb 16, 2015

...69939c84cd69cd/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 line 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 in source code. so I changed it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 89.81% when pulling 2d5cd5c on Bhavik3:issue20 into ff5cbc3 on checkstyle:master.

@rdiachenko
Copy link
Contributor

  • travis build failed
  • code coverage decreased

Please fix

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 89.81% when pulling b728cee on Bhavik3:issue20 into ff5cbc3 on checkstyle:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 89.81% when pulling 870855d on Bhavik3:issue20 into ff5cbc3 on checkstyle:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.9% when pulling 2d6d221 on Bhavik3:issue20 into ff5cbc3 on checkstyle:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.9% when pulling 8031ece on Bhavik3:issue20 into ff5cbc3 on checkstyle:master.

@rdiachenko
Copy link
Contributor

@Bhavik3 please remove changes from .classpath

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 17, 2015

Actually, if i remove changes in classpath, it is showing build error for oraclejdk..

@rdiachenko
Copy link
Contributor

There are no changes in .classpath that is in master and the build passes fine there. That means something wrong on your side. Could you please investigate?

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 17, 2015

I made the same changes as before when everything was fine on previous PR.
even when i don't change in classpath, working fine on my local system. but not working on Travis build (for oraclejdk).

but, i will look at it after reverting changes in classpath.

@romani
Copy link
Member

romani commented Feb 19, 2015

  1. please rebase your code to let me merge easily.
  2. please update comment of commit to DO NOT HAVE LINK in it.
  3. please read http://checkstyle.sourceforge.net/contributing.html#Submitting_your_contribution and please read the whole page - http://checkstyle.sourceforge.net/contributing.html.
  4. I do not see changes to Inputs of UTs - any changes in logic should be reflected in Inputs and UTs.
  5. Please do functional changes as one commit and changes to whole code base as second commit, I need clearly see changes to Check+Input+UTs.

FYI: I need reply "done" or your comment for each of my point. Nothing should be forgotten or skipped.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.88% when pulling efa7abc on Bhavik3:issue20 into 0578116 on checkstyle:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.88% when pulling efa7abc on Bhavik3:issue20 into 0578116 on checkstyle:master.

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 19, 2015

1>> I have removed all links in commit.

2>> UTs:
Actually, there are already, Unit test available for this issue. so, I just update FinalLocalVariableCheck accordingly. no need of additional UTs to add.

3>> coverage is 100% for FinalLocalVaribaleCheck now.

And,

due to stalled build, The build has been terminated.
can you re-run travis build or I would close and again pull request so that Travis build re-run automatically.

@romani

@romani
Copy link
Member

romani commented Feb 19, 2015

commit message should be single line

I am waiting your reply on each my point , I see reply only 3 items from 5

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 19, 2015

  1. please rebase your code to let me merge easily.

done

  1. please update comment of commit to DO NOT HAVE LINK in it.

done

  1. please read http://checkstyle.sourceforge.net/contributing.html#Submitting_your_contribution and please read the whole page - http://checkstyle.sourceforge.net/contributing.html.

read very carefully.

  1. I do not see changes to Inputs of UTs - any changes in logic should be reflected in Inputs and UTs.

Already answered in previous comment.

  1. Please do functional changes as one commit and changes to whole code base as second commit, I need clearly see changes to Check+Input+UTs.

Ruslan told me to squash all my commit into one. so I squashed all my changes into one.
If you want: FinalLocalVariableCheck in one commmit
input (UTs) into one
changes in other related files in third one

Thanks for your support.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.88% when pulling fe5c5fe on Bhavik3:issue20 into 0578116 on checkstyle:master.

@romani
Copy link
Member

romani commented Feb 19, 2015

  1. please split commit into two, one for Check+Uts+Input, second for whole Checkstyle code to follow new Check requirements. It is very hard to do code review on all changes combined.
    Ruslan might mean to have one commit for all functional changes.

FOR_EACH_CLAUSE variable is not reported fixed.reference #20

please make it "fix for FinalLocalVariableCheck - FOR_EACH_CLAUSE variable is not reported. issue #20"

@romani romani changed the title >> https://github.com/Bhavik3/checkstyle/blob/889c7bcfe036c46ad40904ce84... FOR_EACH_CLAUSE variable is not declared final Feb 19, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.88% when pulling 2d29326 on Bhavik3:issue20 into 0578116 on checkstyle:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 89.88% when pulling 2d29326 on Bhavik3:issue20 into 0578116 on checkstyle:master.

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 19, 2015

done
@romani

@romani
Copy link
Member

romani commented Feb 19, 2015

Message for commit should describe update in it, so second commit should be renamed to describe its changes but still have reference to issue.

Please keep reply me "done " to each my point.

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 19, 2015

done
@romani

@romani
Copy link
Member

romani commented Feb 27, 2015

FinalLocalVariableCheck.java

no update for Javadoc is NOT acceptable, one day we will generate xdoc from javadoc

<td>Controls whether to check enhanced for-loop variable.</td>

Please make a link to JDK specification for "enhanced for-loop", not all Checkstyle users are that smart to understand that term, even I was surprised on how it is named actually.

+ private boolean validateEnhancedForLoopVariable;
+ /**

Pelase make empty line between declarations

@Bhavik3 Bhavik3 force-pushed the issue20 branch 2 times, most recently from 09f8e66 to 44d0513 Compare February 28, 2015 16:19
@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Feb 28, 2015

I could not find target file to update javadoc in xdocs folder except config_coding.xml
can you tell me which one is the target file.

DONE

DONE
@romani

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 89.96% when pulling 44d0513 on Bhavik3:issue20 into 385d815 on checkstyle:master.

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 1, 2015

DONE
@romani

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 89.96% when pulling 5d161c2 on Bhavik3:issue20 into 491094e on checkstyle:master.

@romani
Copy link
Member

romani commented Mar 1, 2015

  1. Revert changes at line 35

  2. please provide human description of option in javadoc amd only after that provide example For it.

@Bhavik3 Bhavik3 force-pushed the issue20 branch 2 times, most recently from 3c6c78a to c4fdffe Compare March 1, 2015 22:11
@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 1, 2015

  1. DONE

  2. It is for developers, so I have not explain what is Enhanced For-Loop Variable to make it compact. I have just explained about what is option for it and how to use it.
    DONE

@romani

@romani
Copy link
Member

romani commented Mar 1, 2015

yes it is for developers and in future for xdoc generation so - please do provide example of enhanced FOR and link to specification. Do not expect that checkstyle is used by well educated users.

@@ -39,6 +39,19 @@
* &lt;property name="token" value="VARIABLE_DEF"/&gt;
* &lt;/module&gt;
* </pre>
* <p>
* By default, FinalLocalVariableCheck does not check for Enhanced For-Loop Variable.
* To enable it,there is an option called "validateEnhancedForLoopVariable".It takes boolean value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space between . and It

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 2, 2015

@mkordas
Thanks for correcting me.
DONE

@romani
JAVADOC updated.
DONE

@romani
Copy link
Member

romani commented Mar 2, 2015

Following shows how enhanced For-Loop works for Arrays.

amke javadoc to looks like:

"By default this Check skip final validation on
Option 'validateEnhancedForLoopVariable' could be used to make Check to validate even variable from Enhanced For Loop.

Example:
for (int number : myNumbers) { // violation
System.out.println(number);
}
"

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 3, 2015

@romani
DONE

@romani
Copy link
Member

romani commented Mar 3, 2015

Good, now apply all changes in javadoc to xdoc, as user read results of xdoc and still do not have tool to generate xdoc from javadoc.

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 4, 2015

u mean in config_coding.xml ?
I have already updated it.
@romani

@romani
Copy link
Member

romani commented Mar 4, 2015

You have only changes to option, but all changes in javadoc should appear in xml

@Bhavik3
Copy link
Contributor Author

Bhavik3 commented Mar 6, 2015

DONE
@romani

@romani romani merged commit 93a33df into checkstyle:master Mar 6, 2015
@romani
Copy link
Member

romani commented Mar 6, 2015

you did it !
thanks a lot !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants