SHRINKRES-87: PackagingType is no longer an enum #22
Conversation
|
Probably code formatting yes. I wasn't sure if it should be added to cache, because it could lead to a memory leak, but I can change that anyway |
|
Yes, I tried to minimize the impact on this change by keeping the enum feature. However, I think the best would be to replace with a String |
|
There you go now. There were some failing tests that are not related to my commit (they were failing before the change) |
|
Hi George. First, let me thank you for your effort. I agree with the fact that having and Enum based packaged type is problematic when trying to implement other packaging types such as FAR. However, I don't like removing type safety from the API by moving from a separate type to a simple String. In the future, there is a possibility there will be more functionality related to the packaging type, such as be able to package the project (e.g. emulating maven-${packagingType}-plugin in MavenImporter (such as it was done in https://github.com/shrinkwrap/resolver/blob/1.1.0-X/impl-maven/src/main/java/org/jboss/shrinkwrap/resolver/impl/maven/MavenPackagingType.java) Also, there are few tests failing after your changes, most likely related to == operator used to compare packaging type, which obviously does not work with plain strings. Therefore, I'd like to keep PackagingType as a first class object with most commonly used types as public static instance and a factory method (maybe "of", PackagingType.of("far") seems short enough to me). Implementation note: Cache will likely not be necessary, it's a way how to speed up enum resolution. Could you please revert the changes from commit 44888fc and fix equals methods related to PackagingType? You were almost there. Thanks! |
|
Hi Karel ! I commited your suggested changes. See if it's ok now. As I said before, I have some failing tests (before making the change), but they are not related to the PackagingType change AFAIK. Best Regards, George |
|
What tests are you seeing fail? https://shrinkwrap.ci.cloudbees.com/job/ShrinkWrap_Resolver_upstream-master/ |
|
Failed tests: shouldHaveCentralMavenRepositoryDisabled(org.jboss.shrinkwrap.resolver.impl.maven.integration.DisabledCentralRepositoryTestCase): Expected exception: org.jboss.shrinkwrap.resolver.api.NoResolvedResultException Tests in error: Tests run: 87, Failures: 1, Errors: 1, Skipped: 0 |
|
Looks like we have a test thats dependent upon some env-specific config. |
|
It looks like I could reproduce it. Creating a new JIRA.right now, thanks ! 2012/11/26 Andrew Lee Rubinger notifications@github.com
|
|
JIRA https://issues.jboss.org/browse/SHRINKRES-89 created. Thanks |
|
Hi George, I pushed your changes upstream together with equals() method fix for MavenCoordinateImpl. SHRINKRES-89 is unrelated, afair Andrew hit the same problem some time ago. |
|
Awesome, thanks Em 26/11/2012, às 04:44, Karel Piwko notifications@github.com escreveu:
|
This is the least intrusive change I could think of.
Probably the best way would be to remove this class and use String instead, but I'll leave that as an exercise for the merger :)