kriegaex edited a comment on pull request #83: URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847683826
Yes, `continue` might not look so nice to some people, but is is clearly readable. Code refactoring is always a trade-off. I was taught to avoid nesting control structures as much as reasonably possible. Actually, this could all be factored out into smaller methods, each of which would be more readable. I also do not understand why you prefer to use two tool classes with static methods instead of my suggestion. Static methods are handy sometimes, but usually difficult to test, especially to mock-test. Here, that might not be a big issue, but basically I am not a big fan of utility classes with lots of static methods. In this case here, I was simply aiming for as much a minimal invasive change as reasonable here, in order to avoid lengthy code reviews and enable this thing to be merged ASAP. It is easy enough to fix and has been remained unresolved for too long already. So even if we factor out the possible extension I talked about above - handling classpath directories the same way as JARs with regard to analysing and eliminating/keeping services - into a new issue, this little fix provides customer value already because the warning message is just... suboptimal. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
