Skip to content
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

Merged

Conversation

codecholeric
Copy link
Collaborator

@codecholeric codecholeric commented Mar 6, 2022

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 use case we introduce a new property @AnalyzeClasses(wholeClasspath = true), which is false by default, but can be set to true to restore the old behavior.

Resolves: #719

@codecholeric
Copy link
Collaborator Author

⚠️ As the second part of the description explains, this will still be a breaking change for some users. Thus, we'll release it with 1.0 and we need to make sure to document it carefully as breaking change.

Copy link
Contributor

@t-h-e t-h-e left a 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?

@@ -62,6 +62,12 @@
*/
Class<? extends LocationProvider>[] locations() default {};

/**
* @return Whether to import all classes on the classpath.
Copy link
Contributor

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?

Copy link
Collaborator Author

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@codecholeric
Copy link
Collaborator Author

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 scanPackages(), but just packages(). The mental model was more "analyze classes -> packages", so here I thought "analyze classes -> whole classpath". Do you think users would have a problem to understand this in the context of @AnalyzeClasses(..)?

@codecholeric codecholeric force-pushed the make-AnalyzeClasses-import-test-package-by-default branch from ce3367f to 695af13 Compare March 9, 2022 07:10
@t-h-e
Copy link
Contributor

t-h-e commented Mar 9, 2022

In the context of @AnalyzeClasses it is reasonable and should be understood.
In case of ClassAnalysisRequest all methods start with a verb (all with get for obvious reasons). I'm not a fan of isWholeClasspath, so therefore the suggestion scanWholeClasspath. But this is just personal preference, so do not hesitate to merge 😃

@codecholeric
Copy link
Collaborator Author

In the context of @AnalyzeClasses it is reasonable and should be understood.
In case of ClassAnalysisRequest all methods start with a verb (all with get for obvious reasons)

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>
@codecholeric codecholeric force-pushed the make-AnalyzeClasses-import-test-package-by-default branch from 695af13 to ac29700 Compare March 28, 2022 16:40
@codecholeric codecholeric merged commit 5a2d1e2 into main Mar 28, 2022
@codecholeric codecholeric deleted the make-AnalyzeClasses-import-test-package-by-default branch March 28, 2022 17:50
@codecholeric codecholeric added this to the 1.0.0 milestone Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let @AnalyzeClasses default to scan the package of the annotated class
2 participants