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 #173

Merged
merged 4 commits into from
Jul 29, 2019

Conversation

moboa
Copy link
Contributor

@moboa moboa commented May 1, 2019

Lost push access to toasttab fork in #167 so created this PR.

@codecholeric
Copy link
Collaborator

Sorry for the long break, I had a crazy month at work. I thought about this, and I think the only way it makes sense to me, is to treat anonymous and named inner classes the same. Because conceptually they are almost the same, so I don't see why the rule should be satisfied as long as my inner class has no name and suddenly fail just because I give it a name. This seems to be more driven by a specific use case, that excluding anonymous classes makes the feature really less useful.
I'll try to make a couple of changes in this direction 😃

@moboa
Copy link
Contributor Author

moboa commented Jun 17, 2019

Would creating 3 separate functions areAnyClass, areInnerClassesOf and areAnyOrInnerClassOf be a better solution?

@cakeface
Copy link

I like the multiple function approach.

I get that the behavior is weird between named and un-named inner classes.

In Java 8 and forward lambdas are everywhere. People use them for filtering streams. They use them unknowingly sometimes, or at least the implementation detail of an anonymous class is unknown. I don't like that this implementation detail is leaking up into the archunit tests. But at the same time, those lambdas are another class so I get why it needs to be treated separately if you actually did care about creating rules where inner classes are different.

@ghost
Copy link

ghost commented Jun 17, 2019

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

@codecholeric
Copy link
Collaborator

codecholeric commented Jun 17, 2019

I've added some changes. I would not distinguish here, because we already have the exact case by classes()...should().be(Foo.class), if you want to match just one class.
I think it's okay to count a class and all inner classes as one, because logically this is a group, so I see the use case that you might say "it should be contained within some class, be it directly, in some lambda, anonymous inner class, named inner class or whatever".
I can see myself refactoring, creating new anonymous or inner classes and from the point of my rule this shouldn't matter. Because if I create several nested classes, that is just an implementation detail (okay, if those are public you could argue about it, but it still somehow belongs to the outer class or it wouldn't be an inner class 😉)

@codecholeric
Copy link
Collaborator

Could you check this out if it would match your use case? I don't think it provides any extensive value to exclude named classes as I said.

@moboa
Copy link
Contributor Author

moboa commented Jun 17, 2019

This seems to pretty closely match our use case. Matching against a class and all its inner classes is the best way to go, in my opinion. Could you confirm @cakeface ?

@codecholeric
Copy link
Collaborator

If you don't have any objections, I'll merge this 😉

@moboa
Copy link
Contributor Author

moboa commented Jul 29, 2019

Please go ahead!

mobolajiadefope-toast and others added 4 commits July 29, 2019 02:52
Signed-off-by: Mobolaji Adefope <mbadefop@edu.uwaterloo.ca>
Signed-off-by: Mobolaji Adefope <mbadefop@edu.uwaterloo.ca>
Signed-off-by: Mobolaji Adefope <mbadefop@edu.uwaterloo.ca>
…r classes the same, be they anonymous or named. Thus call the method "belongTo" and define "belong" as being the class or any nested inner class. We should extend that to nested levels as well, since using an anonymous class within an anonymous class should not be counted any different IMHO.

Also improved tests a little to actually test the nested case which is the more interesting one.

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
@codecholeric codecholeric merged commit da47f62 into TNG:master Jul 29, 2019
codecholeric added a commit that referenced this pull request Feb 21, 2021
Add classes().that().areAnyClass(...) to syntax
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.

4 participants