[GitHub] [maven-shade-plugin] kriegaex edited a comment on pull request #83: [MSHADE-366] - "Access denied" during 'minimizeJar'

[GitBox <gi...@apache.org>]

kriegaex edited a comment on pull request #83:
URL: https://github.com/apache/maven-shade-plugin/pull/83#issuecomment-847452890


   As MSHADE-366 was originally created and is still bugging me and because lately @rmannibucau collaborated with me in another PR, I want to restart this one here in order to finally get it off the table. When I checked out @JanMosigItemis's fork and rebased it on master, I noticed that the diff looks bigger than it is due to whitespace changes caused by the `if` wrapped around a big chunk of code. Actually, in we ignore the whitespace changes, it is only this:
   
   ![image](https://user-images.githubusercontent.com/1537384/119423637-ec442900-bd2d-11eb-8ee1-7fd32e05f3ec.png)
   
   As suggested similarly in the Jira issue, I would rather do it like this:
   
   ![image](https://user-images.githubusercontent.com/1537384/119423721-20b7e500-bd2e-11eb-8052-faf7acb0b03c.png)
   
   If would both avoid whitespace changes and make the code more readable by avoiding nesting. It would also avoid two new imports.
   
   I did not look at the test in detail yet, it seems that there was refactored more than necessary in order to test this case, but firstly I could be wrong (going to take a second look) and secondly fixing a bug does not mean we have to avoid refactoring in the surrounding code. Maybe it should just be a separate commit.
   
   The more important question I have is: Is it necessary to do the same analysis and exclusion for used services during minification for files in a `target` directory on the classpath, which currently happens for JAR files, or is it the right thing to just ignore it? Is a classpath directory a valid use case here? It could very well be the case, because the current module's class files are usually part of the uber JAR if not explicitly excluded by a filter. So we would have to mimic the same logic here. I can try doing that, starting a fresh PR superseding this one. But I want a maintainer's qualified answer before I start potentially wasting time with a superfluous feature.


-- 
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:
users@infra.apache.org