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

Add (not)haveFullyQualifiedNameAnyOf to API #1114

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

perlun
Copy link
Contributor

@perlun perlun commented May 26, 2023

Use case: we wanted to do some whitelisting of certain classes for a particular check, deprecating all usage of a given Java package (org.joda.time). We couldn't use existing predicates for this because:

  • Existing checks like areAssignableFrom and areAssignableTo only operate on a single class.
  • Also, assignableFrom is unsuitable for one of our checks since that would implicitly whitelist the base class.
  • There is also belongToAnyOf which would be more correct, but it has to be combined with areNotNestedClasses in that case to avoid running checks on nested classes. (We run both a "normal" test for the whitelisted classes to allow them to use the deprecated package, but also an "inverted" test to ensure classes don't remain whitelisted when they no longer use the deprecated code. I don't remember if this worked or not, but either way, it feels more complex to have to jump through hoops to do an "exact" check for multiple whitelisted classes)

Hence, the suggested new additions to the API in this PR.


assertThatTypes(classes).matchInAnyOrder(ClassAccessedByFoo.class, Foo.class);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests to the best of my knowledge, and as ClassesThatTestsExistTest instructed me. Please let me know if there are any tests missing, I'm not sure this is enough. (feel free to just fix them yourself if it's easier that way 👍)

The tests added are trivial since I could just copy existing single-haveFullyQualifiedName tests.

@perlun perlun force-pushed the feature/add-haveFullyQualifiedNameAnyOf-predicates branch from 024ac91 to babfa5f Compare May 26, 2023 11:06
@perlun
Copy link
Contributor Author

perlun commented Jun 7, 2023

@hankem @codecholeric Please let me know what to do to push this forward. I guess understanding why the tests are failing would be a good first thing. 😄

@perlun
Copy link
Contributor Author

perlun commented Jun 26, 2023

@hankem @codecholeric Ping, please help me push this towards completion. ☺️ 🙇

@hankem
Copy link
Member

hankem commented Jun 26, 2023

I'm really sorry! These are crazy times... it's nothing with your PR, but I was occupied with several other projects that didn't allow me to follow up more with ArchUnit than a bare minimum. I know, it's a bad excuse, but I can only say that you're not forgotten.

@codecholeric
Copy link
Collaborator

Sorry for the late answer! There's just a couple of things coming to my mind.
First, you could already achieve the same by just chaining the existing method with an or(), right? 🤔 (or by using the regex method chaining quoted fully qualified names with a | 😂) Does the varargs method provide sufficient value over that?
Second, if we would add this method, should it then maybe also take Collection? Is the point of the new method to be used programmatically, passing on a set of class names? Then maybe the Collection interface is even nicer than varargs which forces the use of an array?
Third, if we would add this method, should we then deprecate haveFullyQualifiedName(..)? Since the new method would be a superset anyway...
Fourth, wouldn't haveAnyFullyQualifiedNameOf(a, b, c) be more consistent with the existing API?. Because in other places ArchUnit's rule API usually sounds like a "human spoken sentence" (at least to me), like I would explain in a discussion "classes that reside in a package foo should be annotated with bar". I can't picture myself saying "should have fully qualified name any of [foo, bar]", so to me it sounds inconsistent with the pattern that the other methods follow 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants