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 classes().that().areAnyClass(...) to syntax #167

Closed
wants to merge 2 commits into from

Conversation

mobolajiadefope-toast
Copy link
Contributor

Extended the API to allow assertions to made against one or multiple types using class objects.
E.g. The declaration

theClass(Foo.class).should().onlyHaveDependentClassesThat().areAnyClass(Bar.class, Baz.class).check(importedClasses);

should pass if only Bar and Baz and any of their anonymous inner classes depend on Foo.

Signed-off-by: Mobolaji Adefope <mbadefop@edu.uwaterloo.ca>
Signed-off-by: Mobolaji Adefope <mbadefop@edu.uwaterloo.ca>
@codecholeric
Copy link
Collaborator

Hey, thanks a lot 😃 Looks like a reasonable addition. Can you do me a favor though and sign off your commits according to the DCO? -> https://github.com/TNG/ArchUnit/blob/master/CONTRIBUTING.md#commits
With two commits it might be easier to just add the signed-off line (the one you'd get if you use git commit -s) to your messages by rewording.

@ghost
Copy link

ghost commented Apr 10, 2019

DeepCode encountered a problem when analyzing this pull request. If you want to retry, create a comment: "Retry Deepcode".

@mobolajiadefope-toast
Copy link
Contributor Author

Retry Deepcode

@ghost
Copy link

ghost commented Apr 10, 2019

DeepCode analyzed this pull request.
There are no new issues.

@codecholeric
Copy link
Collaborator

One thing that really gives me a stomach ache is the fact that areAnyClass(..) matches inner classes. Cause that does feel specific to your use case and might surprise in other places. The question is, if what you really want is classes with their inner cases, should it be a different name, or is this use case in the end too specific for the general API.
Because for specific things it would be better to use a custom predicate and write sth. like

onlyHaveDependentClassesThat(areAnyOfOrInnerClassesOf(Foo.class, Bar.class))

@mobolajiadefope-toast
Copy link
Contributor Author

It only matches anonymous inner classes. This is mostly to catch lambdas.

@cakeface
Copy link

I think that for areAnyClass(Bar.class, Baz.class) it is more correct and intuitive to include lambdas within the class for architectural checking. I think we should clearly document the contract.

I'm trying to come up with a situation where we want to match the class but not match anonymous inner classes and not coming up with much.

@codecholeric
Copy link
Collaborator

I don't doubt that in this case you probably always want to cover lambdas as well. But inner classes also covers Foo.Bar, what about those? Named inner classes? Possibly static named inner classes? Does static class Foo.Bar count as Foo? Or is it a different class?

@mobolajiadefope-toast
Copy link
Contributor Author

Oops. Did not notice i was matching with non-anonymous inner classes. Will fix this.

@mobolajiadefope-toast
Copy link
Contributor Author

Lost push access to toasttab fork so created #173

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.

None yet

3 participants