Merged
Conversation
Contributor
|
I'm highly interested in getting this PR to be merged. 🙂 Are there any more obstacles to overcome before this can be merged? |
hankem
approved these changes
Aug 15, 2021
Member
hankem
left a comment
There was a problem hiding this comment.
I'm sorry that it took me so long to review this PR!
...a/com/tngtech/archunit/core/importer/ClassFileImporterGenericCodeUnitParameterTypesTest.java
Show resolved
Hide resolved
...a/com/tngtech/archunit/core/importer/ClassFileImporterGenericCodeUnitParameterTypesTest.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/core/domain/properties/HasParameterTypes.java
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/core/domain/AccessTarget.java
Show resolved
Hide resolved
Collaborator
Author
No worries, I know that it's not in all phases of life super easy to spend a lot of time on OSS 😉 You're always giving great support to ArchUnit, so thanks a lot! |
There is actually a violation of the current layer structure, because the method `void setClassResolver(Class<? extends ClassResolver> classResolver)` has a generic parameter type dependency on `ClassResolver` from the `import` layer. I thought about fixing it, but * it is public API * it would make the API harder to use (either no guard, or split up into separate places) * I don't consider this dependency especially harmful for now (the dependency could be removed without breaking any clients at any point) Thus I have decided to add a conscious exception for this case for now. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Note that raw and generic parameter types do not behave the same way in all cases. Specifically for constructors of inner classes that obtain a reference to their enclosing class. In this case the Reflection API returns the enclosing class as first raw parameter of the constructor, while the generic parameters (in case there are any) do not contain the enclosing class. Thus I decided to keep both raw and generic parameters in `JavaCodeUnit` separately, while `getParameters()` will return the raw parameters (and thus one extra parameter for inner classes) in non-generic cases. This does not feel great, but the alternatives seem even worse. At least this way it is consistent to the Reflection API. If we choose a different path there will most likely be even more confusion. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Covers dependencies from and to a `JavaClass` by some generic type argument of a method or constructor. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
79ddd53 to
e646c89
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As last step to fully support Generics within ArchUnit this adds support for generic method/constructor parameter types. In particular
JavaMethod.getParameterTypes()now returns aListofJavaParameterizedTypefor parameterized generic method/constructor parameter types and the raw types otherwiseJavaClass.directDependencies{From/To}SelfExample: ArchUnit would now detect
StringandFileasDependencyof a method declaration likeNote that analogously to the Java Reflection API
JavaConstructor.getParameterTypes()andJavaConstructor.getRawParameterTypes()do not behave exactly the same for inner classes. WhileJavaConstructor.getRawParameterTypes()contains the enclosing class as first parameter type,JavaConstructor.getParameterTypes()does not contain it if the constructor has generic parameters. On the other hand it just returns the same list of raw parameter types if the constructor is non-generic. This might surprise some users, but I decided to stick to the same behavior as the Reflection API, because this has always been the strategy and the solution can never truly satisfy all assumptions a user might have.Resolves: #115
Resolves: #144
Resolves: #440