-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make @AnalyzeClasses
import package of test class by default
#828
make @AnalyzeClasses
import package of test class by default
#828
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Just one nit-picking comment.
Maybe instead of wholeClasspath
-> scanWholeClasspath
or analyzeWholeClasspath
?
archunit-junit/src/test/java/com/tngtech/archunit/junit/ClassCacheTest.java
Outdated
Show resolved
Hide resolved
@@ -62,6 +62,12 @@ | |||
*/ | |||
Class<? extends LocationProvider>[] locations() default {}; | |||
|
|||
/** | |||
* @return Whether to import all classes on the classpath. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe scan
or analyze
instead of import
, to better distinguish the wording from the ImportOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Whether to look for classes on the whole classpath
? I just checked the other Javadocs and that seems consistent to packages()
, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
Thanks a lot for the review 😃 About the naming of the method, I thought it feels consistent with the other methods 🤔 Because we also don't call it |
ce3367f
to
695af13
Compare
In the context of |
Got it 🙂 I'll think about it 👍 |
In particular coming from a Spring framework background it is a somewhat surprising behavior that a plain `@AnalyzeClasses` without any options will import the whole classpath. The more intuitive behavior would be to import the package of the annotated class. Since importing the whole classpath is likely not what most users want, and the performance drain is large, we will change the behavior to import the package of the annotated class by default if no further locations are specified. However, this might break some existing use cases where scanning the whole classpath in combination with `ImportOptions` was used consciously. I.e. something like ``` @AnalyzeClasses(importOptions = MySpecialFilter.class) ``` To make sure ArchUnit can still satisfy this usecase we introduce a new property `@AnalyzeClasses(wholeClasspath = true)`, which is `false` by default, but can be set to `true` to restore the old behavior. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
695af13
to
ac29700
Compare
In particular coming from a Spring framework background it is a somewhat surprising behavior that a plain
@AnalyzeClasses
without any options will import the whole classpath. The more intuitive behavior would be to import the package of the annotated class. Since importing the whole classpath is likely not what most users want, and the performance drain is large, we will change the behavior to import the package of the annotated class by default if no further locations are specified.However, this might break some existing use cases where scanning the whole classpath in combination with
ImportOptions
was used consciously. I.e. something likeTo make sure ArchUnit can still satisfy this use case we introduce a new property
@AnalyzeClasses(wholeClasspath = true)
, which isfalse
by default, but can be set totrue
to restore the old behavior.Resolves: #719