On 12/07/2010 09:39, Marc Guillemot wrote: > I find interesting how some (Mark, you) seem to fight against > introducing checkstyle and precisely defining what should be the rules > although you make exactly this kind of changes.
Then you completely mis-understand my position. My default preference would be to enable every single check I could find and ensure that the code-base passes all of them. However, I recognise that not everyone thinks the same way and that there have been strong objections to this sort of clean up in the past. Therefore, I think it is important to maintain a strong consensus on any global changes in this area and not to try and force them through when some committers are very much against them. I am also of the opinion that whilst there are open bugs currently being experienced by users then fixing these bugs is a higher priority than beautifying the code. Code clean-up is something I tend to look at when I have a spare five minutes and don't have time to look at anything else. There is consensus for spaces versus tabs so there should be no issues there at all. Unused imports and unused code generally are also checks that I haven't seen objections to. > Why a 80 characters limit? Is this a recognized style rule for Tomcat? Historically, yes - although it is frequently ignored. It would be worth a separate thread to see what the consensus view is on increasing this. >> Fixing them can be discussed and done first, >> before enabling any checkstyle nags in the project. > > for me it doesn't make sense. If you agree that tabs should not be used, > then the build should ensure that. If the build doesn't ensure that, > there is no guaranty that tabs won't be introduced again in the future > and therefore fixing them now is nearly lost time. I think the point here is that we don't want to introduce a new check that instantly causes the build to fail. The reasoning is that we then might not spot issues caused by other changes as quickly as we might. The preferred sequence of events is (and we do the same for new bug related unit tests): 1 add check locally 2 fix issues 3 commit issues fixes 4 commit addition of check where steps 3 & 4 happen very close together (and sometimes the other way around). >> I do not mind against checks that already succeed for the existing >> code. Though if they always succeed they are not really useful. > > wrong. Let's take IllegalImport as an example. It checks that some > imports (default to sun.* packages) are not used. It currently pass but > one error from one committer is enough to make them fail (and don't say > that it can't occur ;-)). Agreed. We do need to check that errors don't slip in further down the line. > I'm impressed by the resistance of Tomcat committers against changes > that aim to improve the code quality. The current discussion remembers > me this patch: > "Run the unit tests as part of the build!!!" > (https://issues.apache.org/bugzilla/show_bug.cgi?id=47124) Then I suggest you go and re-read that thread again and remind yourself what the real objections were. > My feeling is that you really have to wake up. We are in 2010! Tomcat's > large user base is surely enough to give you work until you retire but > if you have more ambitious goals, you surely should give a look at other > projects, like Jetty. In this case there is no discussion to determine > who is the winner concerning measures used to ensure code quality! :-( Taking a confrontational approach isn't going to win you any friends and isn't going to help you achieve the changes you want. I haven't gone back over this thread in detail but I don't recall any objections to adding a checkstyle check for tabs in files. I'd suggest focusing on that and once that is in place, then see what support there is for extending the checks that are enabled / extending the tools used (e.g. add FindBugs). Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
