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

Make ArchConditions#createMessage(T, String) public #826

Closed
daniel-shuy opened this issue Mar 4, 2022 · 4 comments · Fixed by #878
Closed

Make ArchConditions#createMessage(T, String) public #826

daniel-shuy opened this issue Mar 4, 2022 · 4 comments · Fixed by #878

Comments

@daniel-shuy
Copy link
Contributor

daniel-shuy commented Mar 4, 2022

ArchConditions#createMessage(T, String) is really useful when creating custom ArchConditions, but because it is private, it is inaccessible. It would be nice if it was changed to public.

@hankem
Copy link
Member

hankem commented Mar 4, 2022

I wonder whether ConditionEvents could offer an all-round carefree package like this:

    public <T extends HasDescription & HasSourceCodeLocation> void addEventWithFormattedMessage(
            T object,
            boolean satisfied,
            String messageContentFormat,
            Object... messageContentArgs
    ) {
        String message = String.format("%s %s in %s",
                object.getDescription(),
                String.format(messageContentFormat, messageContentArgs),
                object.getSourceCodeLocation()
        );
        add(new SimpleConditionEvent(object, satisfied, message));
    }

I'm not entirely happy with that name... addEventWithFormattedMessage doesn't really communicate that the "message content" (or "core message"? Or "custom part"? 🤔) gets the object's description prepended and the source code location appended, right?

@daniel-shuy daniel-shuy changed the title Make ArchConditions#createMessage(T, String) protected Make ArchConditions#createMessage(T, String) public Mar 5, 2022
@daniel-shuy
Copy link
Contributor Author

@hankem yeah it would be pretty difficult to convey that with a single method. I think having 2 methods is still alright, eg.

@Override
public void check(JavaClass javaClass, ConditionEvents events) {
    String message = ArchConditions.createMessage(javaClass, ...);
    events.add(new SimpleConditionEvent(javaClass, ..., message));
}

It would also allow for more flexibility, eg. customizing the message, or using the default created message to create a custom ConditionEvent.

@hankem
Copy link
Member

hankem commented Mar 6, 2022

ArchConditions#createMessage has IMO a similar issue when it comes to communicating what it does – but I guess that it can be okay in any case with an according JavaDoc and/or a better name.

I like the function on ConditionEvents because it might be more discoverable for users than a static helper function on ArchConditions – but we can still have both, so apologies for hijacking your issue with another idea... 😉
Anyways, I've pushed a PoC to demonstrate how ConditionEvents.addEventWithFormattedMessage would simplify ArchConditions to the branch ConditionEvents.addEventWithFormattedMessage, see in particular 3ae3f61.

@fdesande-diconium
Copy link

I'm having a similar issue when I'm trying to use the method PlantUmlArchCondition.adhereToPlantUmlDiagram(URL, PlantUmlArchCondition.Configuration). The interface PlantUmlArchCondition.Configuration is package private so it works fine in Java, but t's inaccesible in Kotlin because of the KotlinJVM.

Please, make PlantUmlArchCondition.Configuration public

codecholeric added a commit that referenced this issue Jul 8, 2022
This adds an ArchCondition to the fluent API to check whether there is any matching transitive dependency. This is a much more efficient variant of `#transitivelyDependOnClassesThat` that can be used to detect transitive dependencies in a huge codebase or to classes in large 3rd-party libraries like the Android SDK. It focuses on detecting all *direct* dependencies of the selected classes that are themselves matched or have any transitive dependencies on matched classes. Thus, it doesn't discover all possible dependency paths but stops at the first match to be fast and resource-friendly.

Sample usage:
```
noClasses().that.resideInAPackage(“de.foo.service..”)
    .should().transitivelyDependOnAnyClassesThat.resideInAPackage("android..")
```

Then this Architecture
![image](https://user-images.githubusercontent.com/17569373/172873445-17111662-30e2-4388-912e-840a105cd2bc.png)

would output the following violations:

```
java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that reside in a package 'de.foo.service..' should transitively depend on any classes that reside in a package ['android..'] was violated (3 times):
Class <de.foo.service.B> transitively depends on <android.I> by [F->E->android.I] in (B.java:0)
Class <de.foo.service.B> transitively depends on <android.I> by [E->android.I] in (B.java:0)
Class <de.foo.service.C> transitively depends on <android.H> by [D->android.H] in (C.java:0)
```

Resolves: #780 
Resolves: #826
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.

3 participants