On Tue, Sep 25, 2012 at 3:22 PM, Gary Gregory <garydgreg...@gmail.com> wrote: > Hi All, > > I happy to see another RC come along! :) This is large code base so I > appreciate that the reports generate a lot of potential work.
I planned to send this email out just over a year ago, shortly after yours, but since we might actually get to release this time let me send it out now for reference. > -1: Unapproved license: > > src/main/java/org/apache/commons/imaging/formats/jpeg/segments/App14Segment.java Fixed. > Fix obvious FindBugs (to me only?): > > - Possible NPE with > NP_IMMEDIATE_DEREFERENCE_OF_READLINE<http://findbugs.sourceforge.net/bugDescriptions.html#NP_IMMEDIATE_DEREFERENCE_OF_READLINE>@ > https://people.apache.org/~damjan/imaging-1.0rc4/xref/org/apache/commons/imaging/formats/rgbe/RgbeInfo.html#103 Findbugs false positive: it believes every method named "readLine()" behaves like BufferedReader's. I've renamed it to "readNextLine()". > - Is this useless or an incomplete impl?: Useless control flow in > org.apache.commons.imaging.palette.PaletteFactory.makePaletteFancy(BufferedImage) > STYLE > UCF_USELESS_CONTROL_FLOW<http://findbugs.sourceforge.net/bugDescriptions.html#UCF_USELESS_CONTROL_FLOW> > 54<https://people.apache.org/%7Edamjan/imaging-1.0rc4/xref/org/apache/commons/imaging/palette/PaletteFactory.html#54> Incomplete impl, fixed now. > - > If we want non-standard class name (starts with a lower case, then Javadoc > would be nice to understand why this is justified: > public static class tEXt extends PngText > public static class zTXt extends PngText > public static class iTXt extends PngText Because that's the case used for PNG field markers. Converted to standard names anyway. > - What about the > RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE<http://findbugs.sourceforge.net/bugDescriptions.html#RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE>? The only reason that value is not null, is because the method providing it hasn't been overridden - and that method is neither private nor final so it can be overridden. Fixed anyway. > - What about the > FE_FLOATING_POINT_EQUALITY<http://findbugs.sourceforge.net/bugDescriptions.html#FE_FLOATING_POINT_EQUALITY>? Probably a false positive (comparison of variables to the maximum amongst themselves) but I worked around it. > - IIRC, the > EI_EXPOSE_REP<http://findbugs.sourceforge.net/bugDescriptions.html#EI_EXPOSE_REP>issues > are just a normal by-product of the design? Please confirm. Yes they are. > - PMD > - do the easy clean ups of "Unnecessary final modifier in final class". Done. > - "Avoid empty catch blocks": Add a comment as to why the block is empty, > for example "This exception cannot be thrown because...". This is important > for anyone who tries to grok the code. The name I give to the exception variable (ignore, cannotHappen) should make it obvious. > - "Avoid unused method/fields/etc": Is this unimplemented code or cruft? > If cruft, it should be removed before we give ourselves unneeded BC > headaches. > > - CDP: lots of those but I do not know the code base enough to tell how > easy it would be to refactor duplications and if refactoring is justified. > > - Code coverage: overall, is not great. > > These packages have 0%.code coverage. > - org.apache.commons.imaging.formats.transparencyfilters > - org.apache.commons.imaging.formats.psd.dataparsers > - org.apache.commons.imaging.formats.psd.datareaders > - org.apache.commons.imaging.icc > > Oddly org.apache.commons.imaging.util has only 17% which I would think > would be higher since it is a util package. Even to get that low test coverage, it already takes 40+ minutes to run "mvn site" due to Corbertura. I've added some tests so it's > 0 now. The real "util" package is org.apache.commons.imaging.common. > - Process > > This might be a case of RM process but when I RM I set the release date in > changes.xml to the date I cut the RC. If the RC passes, then you are done, > otherwise you edit it again for the next RC. Ok thank you. > - Javadoc > > Only 3 out of ~40 packages have comments, which makes it harder to find > your way around. Added a few more package comments, but the API is largely designed to be used from the org.apache.commons.imaging.Imaging class alone. > - Checkstyle is not strict enough > - IMO should always complain about missing { } in blocks. I thought there was no official coding style for commons? But I've added it anyway. > There also a lot of generics warnings (in Eclipse, any IDE will tell you > these days.) Upgrades from Java 1.4 are never fun. These were largely fixed in code, but tests might have a few. > Sorry to make it a long laundry list, but there is only one 1.0 ;) You should see what was already fixed :). > There is probably more of course, but I have to go now... > > Gary Thank you very much Damjan --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
