8000 enable `@AnalyzeClasses` annotation to be used as meta annotation by mathze · Pull Request #1300 · TNG/ArchUnit · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

enable @AnalyzeClasses annotation to be used as meta annotation #1300

Merged
merged 4 commits into from
Jan 19, 2025
Merged

enable @AnalyzeClasses annotation to be used as meta annotation #1300

merged 4 commits into from
Jan 19, 2025

Conversation

mathze
Copy link
Contributor
@mathze mathze commented May 6, 2024

so far users are forced to repeat @AnalyzeClasses annotation an every test class. This cause additional maintenance overhead when common properties (e.g. package structure) changes. To support the DRY approach, @AnalzyeClasses annotation can now be used as meta annotation.

Resolves: #182

@mathze mathze mentioned this pull request May 6, 2024
Closed
@mathze mathze force-pushed the feature/GH-182-allow-analyzeclass-to-be-used-as-meta-annotation branch from 253c79e to f28257a Compare May 11, 2024 12:20
@mathze mathze force-pushed the feature/GH-182-allow-analyzeclass-to-be-used-as-meta-annotation branch from f28257a to 4e75e03 Compare June 25, 2024 17:18
@codecholeric
make ReflectionUtils.getAllSupertypes() private
422d20c 
This method is not used directly from any code outside `ReflectionUtils`,
thus it should be an implementation detail and `private`.
The test coverage of the methods using this transitively seems good enough.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
move getValue below the function where it's called
7fd972f 
... to better motivate why it's there, since it's private.

Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric
migrate ReflectionUtilsTest to JUnit 5
05676d2 
Signed-off-by: Peter Gafert <peter.gafert@archunit.org>
@codecholeric codecholeric force-pushed the feature/GH-182-allow-analyzeclass-to-be-used-as-meta-annotation branch from 4e75e03 to 861bf33 Compare January 19, 2025 00:08
@codecholeric
Copy link
Collaborator

Thanks a lot for your support! 😃 The PR looked really nice to me 👍 I still just made some changes, but that has nothing to do with the quality. In particular:

  • I adjusted the style in some places to be closer to existing code
  • I integrated AnnotationFinder into ReflectionUtils - I have mixed feelings about this, because on the one hand it's nice if not too much is mingled into ReflectionUtils. But on the other hand, it felt a little bit unmotivated why so many actions related to Reflection are in there, just this specific annotation handling is outside. We might need to split ReflectionUtils up at some point if it grows too much, but so far it feels like it's still alright to me
  • Biggest change in logic I did was removing the exception if multiple @AnalyzeClasses are in the hierarchy. While I really like the approach to be defensive and potentially poking users into a mistake they made, there is one reason that brought me to think we should just look for the "first" one and use that no matter what: for JUnit 5 we could just use the AnnotationSupport provided by JUnit Platform and we wouldn't have to maintain any code. As far as we maintain JUnit 4, I guess it's fair to support this feature also there. But, if we one day drop JUnit 4 support, because the user base got really small, then having a stricter behavior than JUnit Platform's own AnnotationSupport would prevent us from using that. Furthermore, I think in most cases this shouldn't be a problem, I can't imagine users going crazy on using the meta-annotation support in whatever nested way, so I think this is more a theoretical confusion. If we really get issues about users being confused, because they shot themselves in the foot with confusing meta-annotation combinations in the same hierarchy, then we can still ponder if we need to do something more. But I assume this just won't happen.
  • I added a section in the user guide 🙂 I think the feature is very nice, so we should make sure users can easily find out about it

In any case, I just went ahead and changed this, I really hope you don't mind! It's mainly because I have a hard time these days to make time to work on ArchUnit (as you might notice by the delay of this review 🙈), so having a lot of round-trips on the review would likely slow down the merge considerably. If there is something really wrong with the final state, we can still fix it in a follow-up.

@mathze @codecholeric
enable @AnalyzeClasses annotation to be used as meta annotation
9e90a3b 
so far users are forced to repeat `@AnalyzeClasses` annotation an every test class.
This cause additional maintenance overhead when common properties (e.g. package structure) changes.
To support the DRY approach, `@AnalzyeClasses` annotation can now be used as meta annotation.

Resolves: TNG#182
Signed-off-by: Mathze <270275+mathze@users.noreply.github.com>
@codecholeric codecholeric force-pushed the feature/GH-182-allow-analyzeclass-to-be-used-as-meta-annotation branch from 861bf33 to 9e90a3b Compare January 19, 2025 00:29
@codecholeric codecholeric merged commit 73dfe66 into TNG:main Jan 19, 2025
21 checks passed
@mathze
Copy link
Contributor Author
mathze commented Jan 19, 2025

Hi @codecholeric,

don't worry, I'm glad that it made it into code :) and I could support. I know maintaining OSS isn't an easy job especially if done in sparetime.
Thank you very much for your effort ❤️.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codecholeric codecholeric codecholeric approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow @AnalyzeClasses to be used as a meta annotation
2 participants
@mathze @codecholeric
0