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

Add method toClassDependencies() to JavaAccess and to SliceDependency #1250

Closed
wants to merge 5 commits into from

Conversation

dpolivaev
Copy link

No description provided.

@@ -34,7 +34,7 @@
* </code></pre>
*
* The semantics of the new condition will be {@code (public OR private) AND haveNameMatching}
* and not {@code public || (private && haveNameMatching)}.
* - and not {@code public || (private && haveNameMatching)}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work for you as well?

--- a/buildSrc/src/main/groovy/archunit.java-conventions.gradle
+++ b/buildSrc/src/main/groovy/archunit.java-conventions.gradle
@@ -15,4 +15,5 @@ tasks.withType(JavaCompile) { Task task ->
 
 javadoc {
     options.addBooleanOption('html5', true)
+    options.encoding = UTF_8.toString()
 }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works too, I reverted my change and applied your suggestion.

* @return all matching class dependencies.
*/
@PublicAPI(usage = ACCESS)
public Set<Dependency> toClassDependencies() {
Copy link
Member

@hankem hankem Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name might be inspired by the previous usage of Dependency.tryCreateFromAccess in JavaClassDependencies (which is no public API). I wonder whether the name getDependencies might be more intuitive? (Same in SliceDependency.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name was chosen as in the existing method com.tngtech.archunit.library.modules.ModuleDependency#toClassDependencies

To be honest, I would use getDependencies otherwise.

I have also considered adding an interface declaring this method. It might be called SomeDependency and implemented by JavaAccess, SliceDependency, ModuleDependency, and Dependency. I decided against it only because I was not sure about its name.

@@ -23,6 +26,9 @@ public void when_the_origin_is_an_inner_class_the_toplevel_class_is_displayed_as
.inLineNumber(7);

assertThat(access.getDescription()).contains("(SomeClass.java:7)");
Dependency dependency = getOnlyElement(access.toClassDependencies());
assertThatType(dependency.getTargetClass()).as("target class").isEqualTo(access.getTargetOwner());
Assertions.assertThat(dependency.getDescription()).as("description").isEqualTo(access.getDescription());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move the test for access.toClassDependencies() to a separate test, as it's not really related to when_the_origin_is_an_inner_class_the_toplevel_class_is_displayed_as_location.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
This reverts commit a95c7c3.

Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
Signed-off-by: Dimitry Polivaev <[email protected]>
@codecholeric
Copy link
Collaborator

As discussed I think this is superseded by #1251. If not, feel free to reopen / re-discuss 🙂

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 this pull request may close these issues.

4 participants