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

[AVRO-2145] Cannot create Javadoc on JDK8+ #334

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@Fokko
Copy link
Contributor

Fokko commented Sep 26, 2018

From Java 8 on the JavaDoc is being linted and it will fail early if there are issues with the JavaDoc: http://openjdk.java.net/jeps/172

This patch will add the missing Javadoc with some stubs. Additionally I updated the docs of some of the public API by hand to make the Javadoc more verbose.

The JavaDoc needs to be updated in the future, but this commit will let the compiler pass the javadoc check step.

Tested locally and it passes the tests. This patch only changes the Javadoc and does not change the logic of the code.

Cheers, Fokko

[AVRO-2145] Cannot create Javadoc on JDK8+
From Java 8 on the JavaDoc is being linted and it will
fail early if there are issues with the JavaDoc.

http://openjdk.java.net/jeps/172

This patch will add the missing Javadoc with some stubs
Additionally I updated the docs of some of the public
API by hand to make the Javadoc more verbose

The JavaDoc needs to be updated in the future, but this
commit will let the compiler pass the javadoc check step

@Fokko Fokko force-pushed the Fokko:AVRO-2145 branch from 200bacf to 3efcddc Sep 26, 2018

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 4, 2018

@cutting I'm happy to help review stuff when I can, but 157 files is a bit much for manual review. Can I help here by confirming I have run the tests locally?

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 4, 2018

I can confirm this passes all the java-related tests.

@cutting

This comment has been minimized.

Copy link
Contributor

cutting commented Oct 5, 2018

I'm not fond of whitespace changes. Are all of these really required to pass JavaDoc lint? In particular, a lot of existing Javadoc comments have content on the first line. When these are moved to a new line it causes the rest of the comment to be re-indented. I'd prefer we don't make that style change here.

@nandorKollar

This comment has been minimized.

Copy link
Contributor

nandorKollar commented Oct 5, 2018

In this case I'd recommend simply turning off DocLint. I remember we did similar things on Parquet, here's how it was turned off there.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 5, 2018

Currently it breaks the dist step.

Personally I would prefer to add the JavaDoc. But if the majority prefers to turn it off, I'm fine with it as well. Beside the missing JavaDoc, I've also corrected a lot of the invalid JavaDoc in this PR.

@nandorKollar

This comment has been minimized.

Copy link
Contributor

nandorKollar commented Oct 5, 2018

I think in some cases it would make sense to fix the Javadoc, but adding obvious comments only for DocLint doesn't give too much value. For example (at least in my opinion) we should keep changes like this, but changes only related to the comment style like this don't give too much value.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 6, 2018

Ok, I'll create a new PR.

@Fokko Fokko closed this Oct 6, 2018

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