Support condition to check how many classes have matched

[bedla]

Hi,
pls take a look this proposal. I did not find rule that I can use to match how many classes has been matched to detect if state of code changed dramatically.
To have something similar to following code snippet.
I am not sure about DSL wording.
Thx
Ivos

noClasses().that()
  .haveNameMatching("foo\\.Bar.*")
  .should()
  .haveMatchedCount(123)
  .andShould()
  .accessClassesThat()
  .resideInAPackage("sapho.service..");```

[bedla]

RandomClassesSyntaxTest is failing, working on it.

[codecholeric]

Thanks for your proposal!! 😃
I do see the use for this, even without any further should() it could be useful to have sth. like

classes().that(followDeprecatedPattern).should().containNumberOfElements(lessThan(10));

Which already brings me to my suggestions

• The wording I'm not sure about either, after some pondering I feel like I would understand containNumberOfElements(x) better from a quick glance (without Javadoc). Because the "sentence" talks about "classes that satisfy sth", not really about matching (even though that is what technically happens, of course)
• I would not only offer a fixed number as argument (like containNumberOfElements(123)), but also an overloaded version which takes a DescribedPredicate<Integer>. That way one could do sth. like in my example above and make sure that a deprecated pattern does not spread any further. I think this would in particular be more resilient against the natural evolution of code.

What do you think? If you don't have the time to implement the predicate version, I can also merge your PR and add that myself, so don't feel pressured!!

Anyway, let me know if you need any help with the RandomSyntaxTest, since it's a little tricky 😉

[bedla]

Thx. I will change wording, yours is much better, and create predicated version (mine was just to show the way :-) ). I will try to do my best with that Random test or ping you with questions later.

[bedla]

changes done, pls take a look ;]

[codecholeric]

Thanks for the update!!

First of all, can you do me one big favor and sign off your commits? -> https://github.com/TNG/ArchUnit/blob/master/CONTRIBUTING.md#commits

If worse comes to worse, you can just squash your commits into one and do git commit -s --amend to create one signed commit, I don't think that would be too bad for this case.

I didn't even expect you to write all those GreaterThan... / LessThan... predicates, but thanks 😃

Some comments:

• Is it possible that you switched less and greater? Because I get a fail of
  classes().should().containNumberOfElements(lessThan(10)) .check(importClasses(String.class)) even though I think this should pass (it's only one class). I think the tests like assertThat(lessThan(4).apply(5)).isTrue(); are invers, since the test expects true, but 5 is not less than 4.
• In the condition ArchConditions#containNumberOfElements you don't really need to copy the classes, since all collections from the API are immutable in ArchUnit anyway. Also I think the message of the event should be something like "There are ${size} elements in classes ${javaClasses}", because now it just repeats the predicate description, which is part of the rule description anyway. The event message should care about the actual state -> how many classes were there, not the predicate again
• LessThenPredicate and LessThenOrEqualToPredicate should be LessThan... 😉
• RandomClassesSyntaxTest is still broken, but I'll happily fix this, I just don't want to make commits on top of yours, before they are signed-off

I hope you're not demotivated by another round of remarks!! I really appreciate your support 😃

[bedla]

Sure, no problem i will fix all the issues :-) . I am happy to help - I like and use you project.

[bedla]

fixed, squashed and signed :] pls fix that random test, i have idea how to fix it but from my perspective it looks like that fix is somewhere in a singleParameterProviders, but i am not sure :] ... you will be faster

[codecholeric]

Thanks a lot for your contribution 😃
It turns out, that the original RandomClassesSyntaxTest works just fine with your recent changes 😉 So I've just reverted your change to the parameter provider.
Other than that, I've added an example and integration test, to show how to use that API.
So once the checks are through, I'll merge this, and thanks again!!

[codecholeric]

Oh, I take it back, guess it takes some more work 😉 Gonna fix it real quick...

[codecholeric]

The problem was, that so far all rules evaluate okay for a set of 0 elements. With your addition

noClasses().should().containNumberOfElements(alwaysTrue())

will fail for 0 imported classes, since the rule want that alwaysTrue() is not satisfied by the number of imported classes.

In the end, it is irrelevant to really check the rule for this test though. The intention right from the beginning was, to validate the description of various rule combinations and ensure, that no weird exceptions are thrown when evaluating / checking the created rule. So I've just relaxed the check line in the test, to either pass or throw an AssertionError. All functional behavior should be tested in other tests, dedicated to the respective rule / condition.

[bedla]

ahh, I see ... nice changes. Thx for merging :-)

[codecholeric]

No prob, thx for your contribution 😄
If you should miss something else, don't hesitate to open an issue or create another PR 😉