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 PlantUmlArchCondition.Configuration public #888

Closed
tmohme opened this issue Jun 14, 2022 · 3 comments · Fixed by #891
Closed

Make PlantUmlArchCondition.Configuration public #888

tmohme opened this issue Jun 14, 2022 · 3 comments · Fixed by #891

Comments

@tmohme
Copy link

tmohme commented Jun 14, 2022

I'm using ArchUnit capability to check rules expressed by PlantUML syntax and I'm generally really happy with this.

There is just one minor issue: I get a Kotlin compile-warning for code that is shown in an example in the User Guide.

classes().should(adhereToPlantUmlDiagram(myDiagram, consideringAllDependencies()))

gives me

Type PlantUmlArchCondition.Configuration! is inaccessible in this context due to: public/*package*/ interface Configuration defined in com.tngtech.archunit.library.plantuml.PlantUmlArchCondition

It took me a moment to understand the root cause: While both functions exchanging the Condition are public, the interface itself is not public but package protected.
It works because one function's result is directly used as parameter for the other (so I don't need to import Condition), but it would fail if I wanted store the intermediate result of consideringAllDependencies() in a typed variable.

It would be nice to get rid of this warning.

@codecholeric
Copy link
Collaborator

Thanks for raising the issue! Yes, this has already been mentioned here #826 (comment)

I'm a little confused why it's a problem in Kotlin and not in Java 🤷‍♂️ (but in Kotlin it also still works if you use the public subclasses, there just is a warning, right? At least for me)

Originally it was intended like this, that you couldn't directly use or implement the base interface as a user, but only the specific three subclasses. I.e. I didn't want this as a public API for users to extend, but instead only use those 3 predefined ones. But maybe that was also a little too restrictive, so I'll look into it if it's fine to just make it public or if I want to refactor something then first 👍

@tmohme
Copy link
Author

tmohme commented Jun 15, 2022

Sorry for the duplicate. I've seen #826 but didn't read down to the very last comment so I assumed it is similar but not exactly the same.

It definitely works with Kotlin (1.6.21) as it is right now. It's just that the Kotlin compiler seems to be a bit pickier/conservative with generating warnings (than javac).

I understand your conflicting goals and I currently have no necessicy to implement the interface in my code.
If you stick with your original design goals, maybe a short notice in the docs would prevent further bothering you with issues like this ;)

For now, I pragmatically suppress the warning for the specific statement with @Suppress("INACCESSIBLE_TYPE").

Thx for your effort.

@codecholeric
Copy link
Collaborator

No worries, the other one is a comment kind of hijacking the other one, so it's not bad to have a dedicated issue 😉 And in general I think it makes sense to get rid of this warning one way or the other so that other users are not confused the same way. I'll figure something out 🙂

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