Allow package info classes to be resolved as a dependency#1565
Allow package info classes to be resolved as a dependency#1565wakingrufus wants to merge 1 commit intoTNG:mainfrom
Conversation
6da5209 to
43288eb
Compare
43288eb to
070d20f
Compare
|
I updated this PR to exclude the package-info classes when calculating package metrics. this fixes the failing tests |
070d20f to
81cfe2d
Compare
|
Happy New Year @codecholeric and @hankem ! |
81cfe2d to
09eb2b1
Compare
09eb2b1 to
ab61e25
Compare
|
Hi again @codecholeric and @hankem. I know I have a few PRs open at the moment, but this one is quite important. I would really appreciate of someone could take a look at this please. |
… class dependency when scanning since TNG/ArchUnit#1565 has not been merged in a timely manner, we need a custom implementation on our end to fix this issue until it is finally merged.
ab61e25 to
63c579b
Compare
… class dependency when scanning since TNG/ArchUnit#1565 has not been merged in a timely manner, we need a custom implementation on our end to fix this issue until it is finally merged.
… class dependency when scanning since TNG/ArchUnit#1565 has not been merged in a timely manner, we need a custom implementation on our end to fix this issue until it is finally merged.
… class dependency when scanning since TNG/ArchUnit#1565 has not been merged in a timely manner, we need a custom implementation on our end to fix this issue until it is finally merged.
fix for TNG#1564 exclude package-info classes from package metrics calculations Signed-off-by: John Burns <wakingrufus@gmail.com>
63c579b to
3b8fda6
Compare
|
I'm very sorry for the late response! I'll try to find more time. 🤞 |
No problem, Thanks! |
There was a problem hiding this comment.
I dislike that the package-info is now included in JavaPackage.getClasses() (even as stub if it doesn't exist, which might be a breaking change for some use cases) and don't understand whether or how all classes in the package are retained when recreating the package in the JavaClass class.
IMO it also is the wrong location for the logic of resolving package info.
Can you explain in more detail which problem you want to solve?
To my understanding, the issue seems to be, that the package-infois not always resolved when you expect it to be loaded, but I'm not sure whether that's limited to loading single classes vianew ClassFileImporter()or whether this also affects the "normal" execution with@AnalyzeClasses` annotation
| } | ||
|
|
||
| @Test | ||
| public void function_getPackageInfo() { |
There was a problem hiding this comment.
looking at the previous tests in this class, I would expect that a test with this name would test
JavaClass.Functions.GET_PACKAGE_INFO, which doesn't exist.
This test doesn't fit the pattern of the other tests.
I'm not sure whether we would want to have that function, as I'm not sure what I should return apart from the package itself
| @@ -0,0 +1,7 @@ | |||
| package com.tngtech.archunit.core.importer.testexamples.packageinforesolution; | |||
There was a problem hiding this comment.
I don't understand this new package name.
is it important that it is subpackage of the package in which OtherClass is defined or shall just be any other package?
| } | ||
|
|
||
| void completePackageInfoFrom(ImportContext context) { | ||
| this.javaPackage = JavaPackage.from(Arrays.asList( |
There was a problem hiding this comment.
I'm not familiar with the class resolution yet, but this looks to me as if the package is re-created with just the current class and the package-info class.
If you look e.g. at the package testexamples, you have these classes (and more):
SomeClassOtherClasspackage-info
assuming we are now in the JavaClass representing SomeClass, I expect that getPackage().getClasses() contains all three of these classes, but I don't see how it could return anything else then Set.of(SomeClass, package-info), as this statement replaces the package and just passes these 2 classes into the new factory.
I don't see any call in JavaPackage.from checking the package of the class, which could contain the information.
It is also not clear to me how the dependencies and subpackages are build if all the other classes in teh package are missing.
Wouldn't it be much cleaner if we could resolve the package-info class when initially building the JavaPackage?
Why should each JavaClass in that package be responsible for resolving the package-info?
| JavaClass otherClass = new ClassFileImporter() | ||
| .importClass(OtherClass.class); | ||
|
|
||
| JavaPackage checkedClassPackageInfo = otherClass.getPackage(); |
There was a problem hiding this comment.
The JavaPackage has the methods tryGetPackageInfo and getPackageInfo.
please include in the test they work as well.
as one calls the other, I would prefer to tests sth like assertThat(checkedClassPackageInfo.getPackageInfo()).isPresent(), even thought this is kind of redundant with the getAnnotations method which also relies on that field.
| JavaPackage checkedClassPackageInfo = otherClass.getPackage(); | ||
| assertThat(checkedClassPackageInfo.getAnnotations()).hasSize(1); | ||
| assertThat(checkedClassPackageInfo.isAnnotatedWith(SomeAnnotation.class)).isTrue(); | ||
| } |
There was a problem hiding this comment.
When running checkedClassPackageInfo.getClasses(), this set now includes the package-info, which it didn't before.
I don't think that that class should be included in that Set as it is not a real class and shall instead be accessed via JavaPackage.getPackageInfo() which returns the JavaClass representing package-info as HasAnnotations. This change in type seems deliberate to me as this special class doesn't have all features of a JavaClass.
|
I can't really localize the issue (and it might be a me-problem), but using windows 11 and IntelliJ, incremental builds fail with this error as soon as I include your changes in |
Allow package info classes to be resolved as a dependency
exclude package-info classes from package metrics calculations
Resolves #1564