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

Provide static and/or/xor methods with varargs #805

Closed
slinkydeveloper opened this issue Feb 21, 2022 · 4 comments · Fixed by #893
Closed

Provide static and/or/xor methods with varargs #805

slinkydeveloper opened this issue Feb 21, 2022 · 4 comments · Fixed by #893

Comments

@slinkydeveloper
Copy link

slinkydeveloper commented Feb 21, 2022

Hi all,
I would love to find out what do you think about adding add(DescribedPredicate, DescribedPredicate, DescribedPredicate...), or(DescribedPredicate, DescribedPredicate, DescribedPredicate...) and xor(DescribedPredicate, DescribedPredicate, DescribedPredicate...).

The reason I ask for those is that in my use case the instance methods and and or lacks expressiveness and clarity of the static method with variadic arguments. Take this example:

return Predicates.xor(
          inFlinkRuntimePackages()
                  .and(containAnyFieldsInClassHierarchyThat(areStaticFinalOfTypeWithAnnotation(InternalMiniClusterExtension.class, RegisterExtension.class))),
          outsideFlinkRuntimePackages()
                  .and(containAnyFieldsInClassHierarchyThat(areStaticFinalOfTypeWithAnnotation(MiniClusterExtension.class, RegisterExtension.class))),
          inFlinkRuntimePackages()
                  .and(isAnnotatedWithExtendWithUsingExtension(InternalMiniClusterExtension.class)),
          outsideFlinkRuntimePackages()
                  .and(isAnnotatedWithExtendWithUsingExtension(MiniClusterExtension.class)));

The way this is structured in the code is much similar to the way i would do it in a predicate, where I have a couple of subpredicates and the root predicate that wraps all of them.

What do you think?

@codecholeric
Copy link
Collaborator

I'm not completely sure I understand 🤔 Why do you think and(x, y, z) is more expressive than x.and(y).and(z)? To me the second feels closer to natural language than the first one 🤷‍♂️ Isn't this more a question of indentation and line breaks in the case of nested predicates?

Other than that where I see an advantage of the static version is for programmatic use (e.g. I have a set of predicates and I want to AND them, because that's IMHO a little more tedious with the instance method version). But in that case I would definitely also offer a version that takes a collection additionally to the varargs, because nothing is worse in Java than these clunky conversions of collections to arrays 🤪

What is not easy to model at the moment is xor, but I also wonder for the concrete use cases 🤔 Because in your case or would probably also suffice, no? I mean, it's probably not relevant for the test that uses this predicate, that it is exclusive? Just from reading this without any background information I wonder, if you e.g. really wouldn't want to match a case where a class in Flink runtime package has a field with the annotation and is also annotated with the extension? I guess this doesn't make sense and should be prevented, but that sounds like a separate rule to me then 🤷‍♂️ Because using the xor you would just hide the problem here, no?

Anyway, as mentioned I would definitely be open for a static and(..) and or(..) if it also covers the collection case 🙂 About xor(..) I'm principally also open, but I'm still wondering about concrete cases. But I could imagine they are out there, even if I can't think of one right away 😉

@slinkydeveloper
Copy link
Author

Hi @codecholeric, the problem with something like x.and(y).and(z) is nesting predicates and associativity in general. How do I express (x and y) or z, x and (y or z), (x or y) and zandx or (y and z)`? And what If I wanna go deeper with the predicates?

Another problem I see with the x.and(y).and(z) approach is associativity: reading from the code, you always do left associativity no matter the predicate, hence x.or(y).and(z) becomes (x or y) and z. Is that correct? If that's the case, it seems a bit unnatural for cases where associativity should be right first, for example the aforementioned x.or(y).and(z), which in my brain translates to x and y or z, should be associated like x or (y and z), as the and has precedence over or usually. This kind of confusing situations could be avoided by using static methods and, or and xor.

In general I think it's good for a predicates DSL to have both styles, as one can pick between the "always left associative" or the nesting case. There is an interesting chapter about it in the DSL book by Martin Fowler (respectively https://martinfowler.com/dslCatalog/functionSequence.html and https://martinfowler.com/dslCatalog/nestedFunction.html).

Because in your case or would probably also suffice, no? I mean, it's probably not relevant for the test that uses this predicate, that it is exclusive?

It is: the exclusivity is important, as i don't want both the annotation and the field, as having both means running twice the extension (from what i know about JUnit 5). I also don't see why i need a second rule, as the xor is exactly describing the situation I want: either one condition or the other, but not both. In other contexts I've also seen the name oneOf (e.g. json schema) to specify that I only want one of these conditions to be true.

@codecholeric
Copy link
Collaborator

You're right that this is implemented as quite simple chaining without any associativity rules. But to me it's still very clear from an associativity point of view if I do e.g.

x.and(y).and(z) == (x.and(y)).and(z)
x.and(
  y.or(z)
)
( x.or(y) ).and( a.or(b) )

I mean, you can add brackets to make it more explicit, right? This seems pretty clear to me that e.g. the last one is (x OR y) AND (A OR B) 🤷‍♂️

Otherwise you would have

and(or(x, y), or(a, b)) == ( x.or(y) ).and( a.or(b) )

In my opinion the and and or instance methods let you nicely express associativity 🤔 (but I agree that you can also write it in a way that looks more confusing if you don't use brackets and chain and and or in an un-intuitive way)

Regarding that xor is exactly what you want, because you want only one condition (predicate?) to be true, wouldn't the problem be, that you just ignore the illegal case then? Say I have something like classes.that(xor(first, second)).should(..) because I say "first and second should never be both true", then the rule will simply not run if first and second actually both should be true 🤔 Isn't that undesired behavior? Wouldn't you rather want a fail? But I might also just be missing your full context and you're using it as part of should where it works out again...

@codecholeric
Copy link
Collaborator

I implemented static factory methods and(..) and or(..) in #893. Regarding xor I decided to leave this out for now. Because for a varargs method I also consider the semantics quite confusing. E.g. should xor join the elements with regards to some associativity rules element by element, or should it be an "extension" of the two elements variant that would behave like "only one element is true/false". IMHO there could be arguments for each interpretation. And doing it something like left-associative feels to me quite complex to understand (e.g. xor(true, false, false, true) -> false). I'm still not convinced that there are really good use cases for xor, where the use case should not actually be solved in a different way. So for now I would go with and and or and leave the implementation of the perfect xor method as an exercise for the user 😬

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 a pull request may close this issue.

2 participants