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

#1500 - Simplify JavaDocs #1630

Closed
wants to merge 4 commits into from

Conversation

jaferkhan
Copy link
Contributor

Hello people,

Here goes my second pull request for this repository.

I know that this is a massive pull request as it covers a lot of files but I've done my due diligence with regards to the change that was asked. Hence, I have only touched those files where this was really required.

It took me four days to sift through the entire codebase. Please let me know if I have made a mistake so I can fix it.

Thanks,
Jafer

@Thihup
Copy link
Collaborator

Thihup commented Mar 28, 2021

Hi! Thanks for the PR.
It looks like the Checkstyle is not enjoying this new syntax, I'll take a look if they already support this feature, otherwise, I'll open an issue in the Checkstyle repository.

@jaferkhan
Copy link
Contributor Author

I checked the CI logs right now and noticed it too. I genuinely thought that this change would not cause any build issues. I should've checked the build on my machine before submitting a pull request but I hope that an update to the newest version of Checkstyle could resolve the issue. I am not sure whether this feature is supported by it right now.

@Thihup
Copy link
Collaborator

Thihup commented Mar 28, 2021

@manorrock
Copy link

@jaferkhan I appreciate all the hard work you have done here. Unfortunately the size of the PR makes reviewing it a nearly impossible task. Can you do multiple smaller PRs, say for each specific language syntax change, for which we then can file separate issues linking to the original? Again, my apologies for not stating this in the original issue.

@jaferkhan
Copy link
Contributor Author

jaferkhan commented Apr 7, 2021

@manorrock Thank you so much for the appreciation. It means a lot. I completely agree with you that this makes it difficult for the team to review the changes; so I should be creating multiple smaller pull requests.

I apologize for causing any confusion but this pull request covers only one language syntax feature, which is the Javadoc return inline tag.

The problem that we are currently experiencing right now is that the build fails with my changes because Checkstyle doesn't support these changes at the moment. @Thihup created an issue in their repository, which can be found here: checkstyle/checkstyle#9745

If it helps, you may find the files affected per module by this pull request:

MODULE				|  FILES CHANGED
jpa				|  02
piranha-api			|  01
piranha-cdi-weld		|  03
piranha-embedded		|  02
piranha-extension-micro-core	|  01
piranha-http-api		|  03
piranha-http-impl		|  01
piranha-http-undertow		|  01
piranha-micro-builder		|  02
piranha-micro-loader		|  03
piranha-monitor-jmx		|  02
piranha-naming-impl		|  04
piranha-naming-thread		|  01
piranha-nano			|  04
piranha-pages-wasp		|  01
piranha-resource-api		|  03
piranha-resource-impl		|  06
piranha-resource-shrinkwrap	|  02
piranha-server			|  01
piranha-server2			|  01
piranha-servlet-api		|  54
piranha-session-hazelcast	|  01
piranha-transaction-api		|  03
piranha-transaction-nonxa	|  04
piranha-webapp-api		|  18
piranha-webapp-impl		|  35
piranha-webapp-impl-test	|  02
soteria-basic			|  01
soteria-form			|  01
wicket				|  01

Please let me know how you would like me to proceed on this.

@manorrock
Copy link

OK we'll need to figure out if we can turn the Checkstyle check off just for this particular usage. @Thihup Any insights here?

@Thihup
Copy link
Collaborator

Thihup commented Apr 9, 2021

@manorrock Looks like adding <property name="allowMissingReturnTag" value="true"/> to the JavadocMethod module of Checkstyle in the pom.xml disable the error. Unfortunately, during my tests, I've hit this Javadoc bug. It would be good to check if this error happens in the Github action too.
But for that, @jaferkhan could you please fix the conflicts? Most of them are related to some classes that were removed

[ERROR] Exit code: 1 - warning: unknown enum constant Resolution.OPTIONAL
[ERROR]   reason: class file for org.osgi.annotation.bundle.Requirement$Resolution not found
[ERROR] javadoc: error - An internal exception has occurred. 
[ERROR]         (java.lang.IndexOutOfBoundsException: Index: 0, Size: 0)
[ERROR] Please file a bug against the javadoc tool via the Java bug reporting page
[ERROR] (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com)
[ERROR] for duplicates. Include error messages and the following diagnostic in your report. Thank you.
[ERROR] java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
[ERROR]         at jdk.compiler/com.sun.tools.javac.util.List.get(List.java:490)
[ERROR]         at jdk.javadoc/jdk.javadoc.internal.doclint.Checker.visitReturn(Checker.java:944)
[ERROR]         at jdk.javadoc/jdk.javadoc.internal.doclint.Checker.visitReturn(Checker.java:105)
[ERROR]         at jdk.compiler/com.sun.tools.javac.tree.DCTree$DCReturn.accept(DCTree.java:701)
[ERROR]         at jdk.compiler/com.sun.source.util.DocTreePathScanner.scan(DocTreePathScanner.java:76)
[ERROR]         at jdk.compiler/com.sun.source.util.DocTreeScanner.scanAndReduce(DocTreeScanner.java:87)
[ERROR]         at jdk.compiler/com.sun.source.util.DocTreeScanner.scan(DocTreeScanner.java:102)

@manorrock
Copy link

Unfortunately the error @Thihup is seeing locally also happens when executing using the workflow. @jaferkhan We probably want to do a new smaller PR with the Checkstyle check turned off and if that works then follow it by a number of smaller PRs.

@jaferkhan
Copy link
Contributor Author

@manorrock,
You may proceed to close this pull request now as all the changes that are available here now live independently in separate pull requests.

@manorrock
Copy link

This was addressed by a series of smaller PRs by @jaferkhan. Closing this.

@manorrock manorrock closed this Apr 16, 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.

None yet

3 participants