-
Notifications
You must be signed in to change notification settings - Fork 306
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
Improve Javadoc of predicates and functions #912
Conversation
Signed-off-by: Peter Gafert <[email protected]>
The type bounds were too limiting. If we want to restrict to `onlyBeCalledByMethodsThat(predicate)`, then we should support a `DescribedPredicate<T>` up to `T` being `JavaMethod` and not just `JavaMember`. Otherwise, it is impossible to assert things like `onlyBeCalledByMethodsThat(haveRawParameterTypes(..))`, because `haveRawParameterTypes(..)` is a predicate on `JavaCodeUnit` and not `JavaMember`. Signed-off-by: Peter Gafert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duly noted that many predefined predicates can be found within a subclass Predicates
of the respective domain object or a common ancestor. 😂 SCNR
Wow, that was a lot of work, but it might probably help the users a lot!
(Let's hope that we'll keep this consistent in the future... 🙈)
I've just left some wording suggestions; they probably won't make much difference, just pick whatever you prefer.
archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldConjunction.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldConjunction.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldConjunction.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldConjunction.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShouldConjunction.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/com/tngtech/archunit/library/dependencies/syntax/GivenSlicesConjunction.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/com/tngtech/archunit/library/dependencies/syntax/GivenSlicesConjunction.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/com/tngtech/archunit/library/dependencies/syntax/GivenSlicesConjunction.java
Outdated
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchPredicates.java
Show resolved
Hide resolved
archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchPredicates.java
Outdated
Show resolved
Hide resolved
Thanks so much for the extensive review!! 😄 I know that it is a lot of repetition and I also worry about keeping it consistent in the future. But I see no other choice, because you don't know from where users enter the API 🤷♂️ And the benefit of having this in many places IMHO outweighs if it should be inconsistent in some place in the future... |
In particular, users were confused about the semantics of joining multiple `andShould()` and `orShould()`. It is not self-evident that there is no operator precedence as developers are used to from `&&` and `||`. We should make sure that every conjunction at least links to some documentation that explains the associativity of joining multiple parts, as well as the alternative by directly supplying predicates/conditions that can be freely nested and joined via `and(..)` and `or(..)`. Signed-off-by: Peter Gafert <[email protected]>
At the moment it is quite challenging to find a certain `DescribedPredicate`, because users might need to know the domain model very well to find the necessary location. While, in the long run we might offer a better entry point into the API that improves discoverability of predicates, as a slight improvement we add javadoc to all predicates linking to respective other predicates that can also be applied to the object. E.g. while before users might be confused why `JavaClass.Predicates` has no predicate defined for `JavaClass.name` now there is at least a link that further predicates that can be applied to `JavaClass` due to inheritance can be found in `HasName.Predicates`. Signed-off-by: Peter Gafert <[email protected]>
Same as for predicates at the moment it is quite challenging to find a certain `ChainableFunction` for a specific domain object. Thus, we should also link to other function declarations where it makes sense. Signed-off-by: Peter Gafert <[email protected]>
210e7e2
to
5a0b1b6
Compare
At the moment it is quite challenging to find a certain predicate or function, because users might need to know the domain model very well to find the necessary location (e.g. predicates like
nameMatching(..)
are defined withinHasName.Predicates
to be reused for everything that has a name, but at the same time users now need to know thatJavaClass
implementsHasName
to use these predicates).While in the long term it might make sense to implement a better entry point API to improve discoverability, for now we can at least improve the documentation to link from more specific predicates to more generic ones. I.e. that I have some chance to discover
...That(DescribedPredicate<? super JavaClass>)
-> "you can find predefined predicates inJavaClass.Predicates
JavaClass.Predicates
-> "due to inheritance you can find further applicable predicates inHasName.Predicates
,HasModifiers.Predicates
, ..."