bodewig commented on pull request #160: URL: https://github.com/apache/ant/pull/160#issuecomment-927281071
Thank you @arturobernalg. I must admit at least I am not a friend of a pull request that touches 41 unrelated files. Some of your changes I'm not really happy with. Broadly * I don't see any harm with a redundant null check. * in your change to DirectoryScanner your change decreases readability for me. Where I could see we are passing on the fast parameter as a method argument I now see a constant and I need to figure out it is true/false because `fast` is. There are similry changes later. * in your change to Tar I don't even see why you are sure the size can never be bigger than TarConstants.MAX_SIZE :-) (and why the replacement r.size() statement is there) * the change to AncestorAnalyzer and the other bcel classes change behavior as we'd now throw on IOExceptions - see the code comment as for why the code is what it is * I don't see why you believe ZipEntry getName() can never return null. Other team members may feel different about the first two points. This is not a "failed review", please consider it a remark. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
