Feature/make arbitrary arch conditions from predicates

[u3r]

This is a partial fix for #855.

I've added two of the methods with some tests.
A couple of points though:

a) I had severe problems, getting this to build (see #437) and test (in IntelliJ lots of tests are executed even if only a few are selected). So formatting might be off

b) I have adapted one architecture check - my reasoning is that just converting from a DescribedPredicate<T> to a ArchCondition<T> is ok without contravariance. It makes stuff easier to diagnose and doesn't loose any power as ArchConditions are already handled contravariantly.

c) I have also added an 'inverse' test but I cannot get that to fail - beats me why.

I'm looking forward to your feedback!

[u3r]

@codecholeric, do you have time for some feedback on this? Thanks!

[codecholeric]

Hey, sorry that it took me so long, work pipeline problems 😬 Thanks a lot for your push here! 😃
I had already thought about something like this as well, but I wanted to go a little different direction. The problem for me with these existing ArchConditions (and the reason why I kept them private) is that the description of the single violations typically does not match the one description to be added to the rule text.
For example assume topLevelClasses() returns a DescribedPredicate:

classes().should().be(topLevelClasses())

The DescribedPredicate will naturally have the description top level classes so it completes the rule text "be top level classes". However, if we use the description of this predicate for the violations it's just awkward:

Class <SomeClass> is not top level classes

These two conditions were tailored in a way that they worked internally, but for me they don't have the quality to make it into the public API 🤔 At least I would want a better violation description somehow.

Sometimes the description of the predicate is also fine, but it would at least be necessary to customize it (as these classes allow internally, exactly for cases like the above).

Originally I had thought of something like a more convenient lambda API on ArchCondition, so it's similar to DescribedPredicate.describe(..) 🤔

E.g.

ArchCondition.from(topLevelClasses())
  .describeViolatedAs(javaClass -> javaClass.getDescription() + " is no top level class")

I.e. have some optional additional methods like describeViolatedAs(Function<T, String>) and describeAllowedAs(Function<T, String>) to customize the descriptions of the single violations. That would probably mean though, that it couldn't be added to the fluent API 🤔 But I'm not sure if there is a good way that doesn't have the problem I mentioned above 🤷‍♂️

Regarding your problems, do you have exactly the problem mentioned in #437? Because according to #437 (comment) it should have been fixed by some upgrade, no? I wonder why it runs on Windows CI without any problem, maybe there the autocrlf is set differently.

[u3r]

Hi @codecholeric,

thanks for the long answer even on a Sunday ;)
I understand your reservations - I need to check how I was going to use them - it seems my rules mach better with those methods.
One alternative to think about is to default to singular handling like so: (pseudocode not checked against api)
anyClass().that(hasSomeProperty()).shouldBe(someThing()) (with anyClass() being equivalent to classes() in all but description.

If this holds for most examples, it would
a) have nice to read output (your point) and
b) have a single short way to re-use predicates as conditions (my goal)

Regarding the other problems, I need to check again on my work machine.

[codecholeric]

The only problem I see about anyClass() or similar is that it's quite confusing. At least I as a user would ponder "why is there classes() and anyClass(), what's the difference", etc. And then we would have theClass(), anyClass() and classes() where the first one is singular and the latter ones behave exactly the same 🤔

[codecholeric]

I would rather go with a way to make the condition creator more customizable. For simple cases the predicate might already be fine, then you don't have to do anything. But for complex cases you should be able to customize it to have correct wording. I also think having it as a factory method on ArchCondition would not be so bad, because that's consistent with DescribedPredicate.

[u3r]

I am still checking in semi-blind, as even rebasing onto latest gives me errors on ./gradlew clean build (using JDK17)
(see buildLog.txt )
Is this also supposed to auto-format? If not AND you are by chance using intellij, would you be open to checking in the formatter settings?)

As for the code:
I added some quick-and-dirty version of what I understood you wanted (not polished or named nicely at all)
a) I've put in the lambda based approach you mentioned
b) I've also put in withViolationString that relies on the predicates description so creating this doesn't get too verbose
c) I am not sure where the plural form of the description is generated and how to test that realistically.
This would fail:

 assertThat (ArchCondition
              .from( JavaClass.Predicates.simpleName( "SimpleName" ) )
              .withViolationString( "does not have" ) )
              .hasDescription( "do not have simple name 'SimpleName'" );

but I don't know if it is something the builder should support or that is generated "around" as the ArchCondition is used.
d) The builder out of ArchCondition.from() is to test a fluent api (no probles so far in the simple case)
Alternatively it could of course be ArchCondition.from(DescribedPredicate, FailureTextDescriptor); I am unsure what is nicer.
e) I quite liked the simplicity of the ArchConditions.be/have versions for their simplicity because at least for me they cover a LOT of cases without the worse readability of the more powerful variants.

[hankem]

even rebasing onto latest gives me errors on ./gradlew clean build (using JDK17) (see buildLog.txt )

I've had a similar issue once (and I can still reproduce it on your branch):
For me, ArchUnit doesn't build when I have a non-empty ~/.gradle/gradle.properties (but I don't know yet why this happens 🤷‍♂️).

If you are by chance using intellij, would you be open to checking in the formatter settings?)

CONTRIBUTING.md links to the develop/ folder. Is its ArchUnit-codestyle-intellij.xml what you're looking for?

[codecholeric]

@u3r I would also guess what @hankem suggested about the gradle.properties. In particular the one inside of the ArchUnit project adds

org.gradle.jvmargs=--add-exports jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED

And AFAIS that was what was necessary at the time to make Spotless work unfortunately 😞

As for your question above, I'm not sure I completely understand. The description of the ArchCondition should be derived 1-to-1 from the DescribedPredicate. So .from( JavaClass.Predicates.simpleName( "SimpleName" ) ) would lead to just simple name "SimpleName". The rest should apply to the single violations, i.e. the details of the test when it fails. Like

Class <com.example.Foo> does not have simple name "SimpleName"
Class <com.example.Bar> does not have simple name "SimpleName"
...

[u3r]

I'll take a look at the fails and suggestions but it will take me a while to get back to this - just as a heads up

[u3r]

I'm working out the technical problems (thanks to @codecholeric and @hankem) Indeed I need the global gradle.properties for the corporate proxy but adding the jvm args worked.

However I am still no satisfied with the api.
I've not yet pushed the code for this. It is meant to discuss the actual syntax:

    @ArchTest
   ArchRule demoRule = methods()
         .should( never(
               ArchCondition.from( HasName.Predicates.nameEndingWith( "s" ).forSubtype() )
                     .withViolationString( "has a" ) // customize single violation text (this one uses the original description)
                     /* This next section is needed, because we have no way to customize the original predicate description "name ending with 's'"
                        This is especially bad as you need to repeat the original description again creating noise during rule definition
                      */
                     .as( "have a name ending with 's'" )
         ) )
         .because( "We don't want that" );

   @ArchTest
   ArchRule demoRule2 = methods()
         .should( never(
               ArchCondition.from( ArchPredicates.have(HasName.Predicates.nameEndingWith( "s" )) ) // no need for (further) description customization
                     //again we need to repeat the original description again (as now it's prefix does not match) creating noise during rule definition
                     .describeViolatedAs( entity -> entity.getDescription() + "has a name ending with 's'" )
         ) )
         .because( "We don't want that" );

   @ArchTest
   ArchRule demoRule3 = methods()
         .should( never(
               ArchCondition.from( HasName.Predicates.nameEndingWith( "s" ).forSubtype() ) // how to document that ArchPredicates.have and friends may not be used
                     .withDescriptionPrefix("have a") // prefix for the predicate description (reusing that)
                     .withViolationPrefix( "has a" ) // prefix for the predicate description (reusing that) in the violation case (singular)
         ) )
         .because( "We don't want that" );

   @ArchTest
   ArchRule demoRule4 = methods()
         .should( never(
               ArchConditions.have( HasName.Predicates.nameEndingWith( "s" ).forSubtype() ) // how to document that ArchPredicates.have and friends may not be used
         ) )
         .because( "We don't want that" );

demoRule has a minimal ArchConditionBuilder but is not nice to handle
demoRule2 just 'inverts' the badness of demoRule
demoRule3 would be ok from a usage point of view - still very verbose in comparison to demoRule4 but the methods could be added internally using demoRule3
demoRule4 was my initial proposal (which I personally still like best due to it's brevity while handeling 70% of (my) use-cases with just have() and be() methods.

Before I start spending time with cleanup, naming and such please have a look and let's agree on
what approach to take and maybe what documentation to add, so a user can more easily navigate the pitfalls shown in (demoRule 1 + 2)

My personal favourite would be to implement something that supports demoRule3-like syntax AND the ArchPredicates.have() and .be() methods using that API to provide a 70% case with just one line of code (instead of 3)

[codecholeric]

I'm against adding something to the official API that works nicely in 70% of the cases and badly otherwise. I think it's fine doing this as a user, but I'm not convinced of adding such a thing to the core for all to use. I can see the argument about the bloat, but for me that doesn't trump that an API should work 100% of the cases 🤷‍♂️

My ideas would be

a) instead of ArchCondition.from(..) let's use ArchCondition.have(..) and ArchCondition.be(..) as suggested by you, but make it customizable if necessary. I.e. allow optionally to define something like

ArchCondition.be(INTERFACES)
  .describeEventAs((clazz, isSatisfied) -> 
    clazz.getDescription() + (satisfied ? " is " : " is no ") + "interface")

Then the case where it just works would simply be should(ArchCondition.have(nameEndingWith(..))), cause you could omit the customization, but you can write correct event messages in all cases by customization.

b) To the fluent API I would not add should().have/be(..), because there it is not customizable and only works in 70% of the cases. What I could imagine (if we really want to offer something like this right in the fluent API?) would be a more generic form like should().satisfy(predicate). Which would then cause Class <..> satisfies <simple name ending with 'Foo'> or something like that. Because at least then it's a grammatically correct and meaningful sentence, even though it doesn't read as "natural language" as the usual ArchUnit rules.

[codecholeric]

To move forward in smaller steps I have taken your PR, extracted a part from it and fixed it up a little here: #904
Because for me that part is non-controversial, that we want some convenient factory method to create conditions from predicates. After that we can decide how to move on with this PR here, i.e. should we transform it into something that adds fluent API or more convenience methods on top, or is it alright as it is 🤔
@u3r What do you think about #904? Does it look reasonable to you in its current state? I would be happy about a review 🙂

[u3r]

converted to #904, no more use for this