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 API methods to JavaPackage to read package-info.java #290

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

rweisleder
Copy link
Contributor

Resolves #263

@ghost
Copy link

ghost commented Jan 4, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.385 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

☺️ If you want to provide feedback on our bot, here is how to contact us.

@rweisleder rweisleder changed the title Add API methods to read package-info.java Add API methods to JavaPackage to read package-info.java Jan 4, 2020
@remal remal mentioned this pull request Jan 9, 2020
@codecholeric
Copy link
Collaborator

Thanks a lot!! 😄 Now there is only one problem, with #171 merged, JavaAnnotation now has a type parameter of the type of the element that is annotated. Since package-info is a "class" for ArchUnit and HasAnnotations now also wants the type of the annotated element, this feels a little inconsistent. In particular because one would expect that JavaPackage.getAnnotations() would return JavaAnnotation<JavaPackage>, but at the moment it could only return JavaAnnotation<JavaClass>.
Have to ponder a little how to implement this best, I guess. In particular whether it should be moved closer to the core where JavaAnnotation<JavaPackage> could be created, or if it is actually okay like this (or JavaAnnotation<?> would also be sufficient as a return type, don't know. But then it's maybe still strange to get the JavaClass<package-info> as the JavaAnnotation.getOwner().
What's your opinion here?

@rweisleder
Copy link
Contributor Author

As a user of the API, I would expect

JavaAnnotation<JavaPackage> a = javaPackage.getAnnotationOfType(PackageAnnotation.class);
JavaPackage owner = a.getOwner(); // same as javaPackage

And it feels strange to handle package-info as a class because it's not really a class. We should get rid of this:

this.packageInfo = tryGetClassWithSimpleName("package-info");

@rweisleder
Copy link
Contributor Author

rweisleder commented Jan 24, 2020

If we wanna break the user's code we could make the methods of HasAnnotations consistent. 😉
Currently, we have this:

@PublicAPI(usage = ACCESS)
<A extends Annotation> A getAnnotationOfType(Class<A> type);
@PublicAPI(usage = ACCESS)
JavaAnnotation<? extends SELF> getAnnotationOfType(String typeName);
@PublicAPI(usage = ACCESS)
<A extends Annotation> Optional<A> tryGetAnnotationOfType(Class<A> type);
@PublicAPI(usage = ACCESS)
Optional<? extends JavaAnnotation<? extends SELF>> tryGetAnnotationOfType(String typeName);

Some methods return JavaAnnotation and other methods with the same name return Annotation. So my example above wouldn't even compile.

`Some.class.getName()` is better to maintain than using some inline string for the class name. Also rename `notAnnotatedFoo` -> `nonAnnotatedFoo` since it sounds better to me for combined nouns. Use `JavaPackage.getDescription()` where possible in messages to be consistent.

Signed-off-by: Peter Gafert <[email protected]>
Copy link
Collaborator

@codecholeric codecholeric left a comment

Choose a reason for hiding this comment

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

Hey, thanks again 😃 I've rebased against master and fixed the incompatibilities. The API looks okay for me. Whether package-info.class is a JavaClass or not is a matter of perspective to me. It is compiled to bytecode and it shares some properties, but yes, semantically I would agree.
Still this would be a major refactoring to change this, so I don't know if it's worth it at the moment, since the API looks okay (it should be consistent in either talking about untyped HasAnnotations<?> packageInfo or JavaAnnotation<JavaPackage> where the original package is the owner.
Can you check out the current state if that is okay for you, too?

@rweisleder
Copy link
Contributor Author

Yes, looks good to me 👍 thanks, @codecholeric

@codecholeric
Copy link
Collaborator

Closing and reopening to trigger a new Travis build (the old one seems stuck)

@codecholeric codecholeric reopened this Apr 8, 2020
@codecholeric codecholeric merged commit 7c913ed into TNG:master Apr 8, 2020
@rweisleder rweisleder deleted the gh-263 branch April 8, 2020 16:42
codecholeric added a commit that referenced this pull request Feb 21, 2021
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.

Add rules for packages
2 participants