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 rules for packages #263

Closed
remal opened this issue Nov 12, 2019 · 9 comments · Fixed by #290
Closed

Add rules for packages #263

remal opened this issue Nov 12, 2019 · 9 comments · Fixed by #290

Comments

@remal
Copy link

remal commented Nov 12, 2019

I'd like to check that every package has package-info.java with NonNullApi and NonNullFields annotations:

@NonNullApi
@NonNullFields
package com.moex.mp.sys.payment.libs.commontest;

import org.springframework.lang.NonNullApi;
import org.springframework.lang.NonNullFields;

Currently I can do it only via reflection, not via bytecode.

Please add a possibility of checking packages' annotations.

@codecholeric
Copy link
Collaborator

Are you sure that this doesn't work? I've used the package-info in the past and it worked, if it got compiled. Have you tried something like

classes().that().haveSimpleName("package-info")
    .should().beAnnotatedWith(NonNullApi.class)
    .andShould().beAnnotatedWith(NonNullFields.class)
    .check(classes);

@remal
Copy link
Author

remal commented Nov 20, 2019

Thank you for providing me with very simple snippet! Is it possible to force developer to create package-info?

@codecholeric
Copy link
Collaborator

In theory yes, but you would need to define, where a package-info is mandatory (or would you really want a strict convention for every package? But what about packages like mybusinesscase1.mysubcase.mylayer.utils or similar?
One way to check all packages would be

@ArchTest
public static final ArchRule all_packages_should_contain_a_package_info =
        all(packages()).should(containAPackageInfo());

private static ClassesTransformer<JavaPackage> packages() {
    return new AbstractClassesTransformer<>("packages") {
        @Override
        public Iterable<JavaPackage> doTransform(JavaClasses classes) {
            return classes.getPackage("my.root.package").getAllSubPackages();
        }
    };
}

private static ArchCondition<JavaPackage> containAPackageInfo() {
    return new ArchCondition<>("contain a package-info") {
        @Override
        public void check(JavaPackage javaPackage, ConditionEvents events) {
            if (!javaPackage.containsClassWithSimpleName("package-info")) {
                String message = String.format("Package '%s' does not contain a package-info", javaPackage.getName());
                events.add(SimpleConditionEvent.violated(javaPackage, message));
            }
        }
    };
}

If you only want certain packages (e.g. top-level packages), you would have to filter the packages to analyse.

Since you talk about "force the developers", I usually rather talk about "support the developers to remember what they decided" 😉. Because usually only conventions that are really accepted and supported by the vast majority of the developers work. For example, if you want to enforce a package-info to enforce documentation, you might well end up with

/**
 * Autogenerated package-info
 */
package my.root.package.mybusinesscase1.mysubcase.mylayer.utils;

Which then has little value. Just my two cents 😉

@remal
Copy link
Author

remal commented Nov 21, 2019

Thank you for such beautiful examples! I successfully managed to write it, but in another, much uglier way.

I really like your position on agreements and decisions. I believe that some moments must be discussed, like coding style, but some decisions should be made by the lead himself. Having non-null annotations really simplifies maintenance, but you'll see it only in a long run.

Regarding the issue... Can I ask you to consider extending JavaPackage with methods like isAnnotatedWith and so on? What do you think?

@codecholeric
Copy link
Collaborator

Sorry for the delay! I like JavaPackage.isAnnotatedWith() 😄 What do you have in mind with "and so on"? Maybe haveAPackageInfo() would be useful as a general method?
I'll make this open for grabs, should be fairly simple (just look for package-info within the current package and check for an annotation)

rweisleder added a commit to rweisleder/ArchUnit that referenced this issue Jan 4, 2020
rweisleder added a commit to rweisleder/ArchUnit that referenced this issue Jan 4, 2020
@remal
Copy link
Author

remal commented Jan 9, 2020

I looked at #290 and it looks really nice. Would love to have it merged!

What do you think about adding some rules like all packages should be annotated by ...?

@codecholeric
Copy link
Collaborator

We could add a simple entry point to the API, like packages().should().beAnnotatedWith(..) or similar 😃

codecholeric pushed a commit to rweisleder/ArchUnit that referenced this issue Apr 7, 2020
codecholeric pushed a commit to rweisleder/ArchUnit that referenced this issue Apr 7, 2020
codecholeric pushed a commit that referenced this issue Feb 21, 2021
codecholeric pushed a commit that referenced this issue Feb 21, 2021
@royteeuwen
Copy link

I'm trying to achieve the following with the new API's but I'm not sure on what to do

What I'm trying to do is the following (pseudo code):

@ArchTest
private static final ArchRule packagesShouldBeAnnotated = 
packages()
.that()
.resideInAPackage(ROOT_PACKAGE)
.should()
.haveAPackageInfo()
.and()
.should()
.beAnnotatedWith(Annotation.class)

Can this already be done?

@hankem
Copy link
Member

hankem commented May 10, 2023

@royteeuwen As of ArchUnit 1.0.1, there is no fluent API for JavaPackage-based ArchRules yet. I'd probably do something like

@ArchTest
void packagesShouldBeAnnotated(JavaClasses classes) {
    var rootPackage = classes.getPackage(ROOT_PACKAGE);
    var violations = rootPackage.getSubpackagesInTree().stream()
            .filter(pkg -> !pkg.isAnnotatedWith(ANNOTATION))
            .map(pkg -> pkg.getDescription() + " is not annoted with @" + ANNOTATION.getSimpleName());
    assertThat(violations).as("violations").isEmpty();
}

(Note that getSubpackagesInTree does not include the ROOT_PACKAGE itself. If you wanted that, I'd replace the stream with Stream.concat(Stream.of(rootPackage), rootPackage.getSubpackagesInTree().stream()).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants