To be fair to Gary, he has been putting in a lot of effort to try to improve the code quality, and this particular thing is something that was discussed and agreed on in the mailing list. (I am agnostic on the use of final, btw.)
Ralph, I understand your annoyance at losing your changes (but it sounds like an IDE issue). By the nature of Gary's changes it means making small fixes to many files. I wasn't aware of any policy of giving warning before touching many files, but if that would be useful we can adopt this as a group, I would not mind. Nick, your comments on timing make sense, but personally I have trouble planning in advance when I can spend time on Log4j. When I do finally have time I go ahead and clear up as many things as I can from my to-do list. (And it is not like we have a QA team whose work would be invalidated because we made changes after they finished testing.) (And here is where I insert a joke that makes everyone relax again, but it seems like my correspondence course in how to be funny is not paying dividends... I can't think of anything. I will ask my money back!) Remko Sent from my iPhone On 2013/07/10, at 10:13, Nick Williams <[email protected]> wrote: > I see. > > I imagine we could have this discussion every time a new developer comes on > board. :-) > > So, I see the points they're making, and I don't necessarily disagree with > all of them ipso facto. I, personally, always try to make instance variables > final when appropriate. However, I also think that each developer knows best > the intent of his code, so if we want to put this "always use final when > possible" practice into place, the best way to do that is to train developers > to use it. Put it in the developer guidelines for contributors and > committers, etc. This blanket changing of dozens of files that the changer > has no knowledge of and isn't even reviewing individually concern me. > > And that doesn't even address the fact that these changes increase the > Checkstyle errors. The additional of the six characters per variable (in a > method with many parameters, that's a lot of characters) causes some lines of > code to exceed the 120 character limit and trigger Checkstyle. > > Finally, even /if/ I were to agree to blanket changes like this, I think they > were done at the wrong time. At $work, we have a policy about such changes: > Blanket reformats and policy changes are always made at the /beginning/ of a > development cycle (immediately after a release) as opposed to at the /end/ of > a development cycle (immediately before a release, as GG has done with MANY > changes in the last few days). While such changes /shouldn't/ change program > logic, tools (Eclipse) are only as good as the people who write them (don't > get me started on my opinion of Eclipse) and can make mistakes. We shouldn't > be doing things like this right before a release. > > My $0.02. > > Nick > > On Jul 9, 2013, at 7:14 PM, Ralph Goers wrote: > >> This topic was covered in an email thread that started with this email - >> http://mail-archives.apache.org/mod_mbox/logging-log4j-dev/201301.mbox/%3CCACZkXPwsDyD-X%2B5wHuq4X26wzCZnsHJ%3Ds5rLJNKe71XYyg6LfA%40mail.gmail.com%3E. >> Unfortunately I don't know how to link the whole thread from the apache >> archives but you can find it at http://marc.info/?t=134978818500007&r=1&w=2. >> >> I happen to agree with you, as you will see from the thread. I find it more >> annoying that many, many files were changed at once with no warning. I also >> suspect Gary does this through something automated in Eclipse, which means >> he doesn't see the comments that say "don't do that". >> >> Ralph >> >> On Jul 9, 2013, at 4:41 PM, Nick Williams wrote: >> >>> Gary, >>> >>> Earlier today you committed r1501104 with the following comment: >>> >>>> Add final modifier to private fields. >>>> Add final modifier to method parameters. >>>> Add final modifier to local variables. >>> >>> I've seen you do similar things before, too. Can you explain a little bit >>> why you do this? There's certainly no compiler value--the compiler for a >>> long time has been capable of determining whether a variable is ever >>> changed and optimizing appropriately. All it seems to do to me is clutter >>> the code...significantly. It makes the code harder to read, IMO. >>> >>> Can you justify the intentional mass change of this nature? >>> >>> Thanks, >>> >>> Nick >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: [email protected] >>> For additional commands, e-mail: [email protected] >
