-
Notifications
You must be signed in to change notification settings - Fork 19
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
Resolves #61. Inherit missing javadoc components from overridden methods #64
Conversation
2c04118
to
32d22a7
Compare
…n methods if they exist.
32d22a7
to
0ab754c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. I commented on a few superficial issues. I'll work through the rest of the changes as time permits.
} | ||
|
||
private static ClassJavadoc getSkinnyClassJavadoc(String qualifiedClassName, ClassLoader loader) { | ||
System.out.printf("Getting %s%n", qualifiedClassName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove all calls to System.out.printf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep those should have been removed. Done
ClassJavadoc createEnhancedClassJavadoc(Class<?> clazz) { | ||
if (!getName().equals(clazz.getCanonicalName())) { | ||
throw new IllegalArgumentException( | ||
String.format("Class `%s` does not match class doc for `%s`", clazz, getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STYLE NIT: Please replace all calls to String.format
with plain old string concatenation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
public class MethodJavadoc extends BaseJavadoc { | ||
public static final String INIT = "<init>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer for the INIT
constant to not be part of the public API. Please can you move it to RuntimeJavadocHelper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return INIT.equals(getName()); | ||
} | ||
|
||
public boolean fullyDescribes(Method method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Made package private.
@dnault Have you had a chance to look anymore at this? |
Hi @Sheikah45. Thanks for the reminder. Hey @bbottema, would you be interested in handling this review, by any chance? |
@Sheikah45 Review complete! Thanks for your patience. I imagine this PR took a lot of work, and I really appreciate it. |
@dnault do you have an idea when this might be included in a release? It would be helpful on one of my projects. |
@Sheikah45 Version 0.14.0 with your new feature was released today, and should be available in Maven Central shortly. Thank you! |
This adds javadoc from the overridden method in super class or interfaces if none is present on the declared method at runtime.
When a class is extended and/or multiple interfaces are implemented with the same signature priority returned by the super-class first then the interfaces in the order they are declared as is done by the javadoc command line tool
One of the potentially main issues I was not able to figure out was how to preserve the parameter order when inheriting java doc when some of the params are on the overriding method and some on the overridden. We don't have guaranteed access to the param names at runtime which makes it difficult to discern true order. Currently the inherited params are just added to the end of the list.
If the java doc is not complete then the missing parts are inherited from the overridden methods as described here https://docs.oracle.com/en/java/javase/17/docs/specs/javadoc/doc-comment-spec.html.
When loading a class javadoc all the methods are properly populated with inheritance. When loading just method javadoc only the necessary javadoc are loaded.
I changed the class javadoc to use a map in order to have more efficient method lookup. I left the getters to return list for the various components and the order is preserved from how they are inserted. It may be better to at some point change the return type to a collection as it can be a strict view, but at the moment this was not done in case dependent code expects a list.
Also something to consider is that in the annotation processor we erase all the type parameters so the param types of methods are limited to their bounds which can be seen on the generic method in Documented Class. However the javadoc tool actually preserves the generic type and bounds.
This did not cover the case of adding the javadoc from protected fields or parsing @ inheritdoc as I think these are a separate concern.
Let me know if you want any changes or see any holes.
Note this implementation does not require @ Override to be present on the method.
Note that when you generate javadoc for the VeryComplexImplementation it actually does not follow the algorithm provided [in the link.](https://docs.oracle.com/en/java/javase/17/docs/specs/javadoc/doc-comment-spec.html. It actually performs recursive search on the superclass first resulting in inheriting the javadoc for the fling method from DocumentedInterface rather than CompetingInterface as would be expected if the algorithm ran as described. So likely the order priority is something that changes with java versions unfortunately
Closes #61