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

ARTEMIS-5314 overhaul JavaDoc formatting & content #5512

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbertram
Copy link
Contributor

@jbertram jbertram commented Feb 19, 2025

I apologize in advance for this PR. I started down this road, and it just kept growing. I didn't realize how many JavaDocs we actually had throughout the code-base.

Couple of things to note:

  • There are no semantic changes here. This is just comments.
  • I've set it to draft as there's no urgency to get this into 2.40.0.
  • All in all this PR removes right around 10,000 lines from the code-base which I think is a fairly significant improvement.

@jbertram jbertram marked this pull request as draft February 19, 2025 21:21
@gemmellr
Copy link
Member

gemmellr commented Feb 20, 2025

Someone elses turn to chew on the massive code/javadoc twiddling, I wont be reviewing it :)

Though on the commit message, the https://bugs.openjdk.org/browse/JDK-8256804 CSR Jira seems like it does a far better job of explaining {@return ...} than the decade old dev Jira referenced does, given the latters description doesnt actually mention the correct syntax or effect at all, and the comments don't appear to outline the final state either.

Presumably a draft because of the compilation failure on 21+

@jbertram jbertram force-pushed the ARTEMIS-5314 branch 2 times, most recently from 8a074cf to 1e8c9da Compare February 20, 2025 15:02
@jbertram jbertram marked this pull request as ready for review February 20, 2025 15:30
@@ -16,8 +16,6 @@
*/
package org.apache.activemq.artemis.api.core;

/**
*/
// XXX
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed some extra "comment" that could also get deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@jbertram jbertram force-pushed the ARTEMIS-5314 branch 2 times, most recently from 6c5e762 to 1e09236 Compare February 20, 2025 16:55
@jbertram
Copy link
Contributor Author

The full test-suite is green on this.

@@ -51,7 +51,9 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

/* at the time this test was written JournalFileImpl was leaking through JournalFileImpl::negative creating a linked list (or leaked-list, pun intended) */
/* At the time this test was written JournalFileImpl was leaking through JournalFileImpl::negative creating a linked
Copy link
Contributor

Choose a reason for hiding this comment

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

Could perhaps add a newline after the opening '/*' to bring this into compliance with all the other formatting changes.

@@ -53,7 +53,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/* at the time this test was written JournalFileImpl was leaking through JournalFileImpl::negative creating a linked list (or leaked-list, pun intended) */
/* At the time this test was written JournalFileImpl was leaking through JournalFileImpl::negative creating a linked
Copy link
Contributor

Choose a reason for hiding this comment

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

Should perhaps add a newline after the opening '/*' to be consistent with other changes.

OBJECT, STRING, NUMBER,
TRUE, FALSE,
NULL
ARRAY, OBJECT, STRING, NUMBER, TRUE, FALSE, NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be javadoc related

@jbertram jbertram force-pushed the ARTEMIS-5314 branch 4 times, most recently from d9172ec to cb2deff Compare February 21, 2025 18:00
This commit overhauls JavaDoc formatting & content across the entire
code-base placing a heavy emphasis on the don't-repeat-yourself
principle (i.e. DRY).

The main goals here are readability, conciseness, and safeguarding
future commits from the same kinds of mistakes that have plagued the
code-base for many years now.

It includes the following changes:

 - Remove empty @param, @throws, & @return tags.
 - Remove useless/boilerplate comments.
 - Re-wrap to 120 characters. Many comments already used this limit
   while others were less than and still other were more than this
   limit. Using 120 was a happy medium.
 - Enforce multi-line syntax, e.g.:
   /**
    * Lorem ipsum
    */
 - Change /** to /* where appropriate (e.g. license headers,
   intra-method comments, etc.)
 - Change /* to /** where appropriate (e.g. method descriptions,
   comments using tags like @code, etc.)
 - Remove trailing or extraneous '*' characters.
 - Remove JavaDocs from implementations when the JavaDocs of the
   interface will suffice.
 - Use {@code Lorem ipsum} syntax where appropriate.
 - Use <p> for empty lines. Remove <br>, <br/>, & <br />
 - Use the {@return ...} sytanx introduced in Java 16. Details are at
   https://bugs.openjdk.org/browse/JDK-8256804.
 - Align @param descriptions.
 - Align @throws descriptions.
 - Add empty line between description and start of tags (e.g. @param).
 - Fix the prolific "whether or not" to the more correct just "whether".
 - Enforce many of these standards via checkstyle.
 - Remove references to defunct issue trackers or forums.
 - Convert single line comments using /* syntax to use //.
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.

3 participants