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

[BEAM-6679] Fix javadoc @ literals #7855

Merged
merged 1 commit into from Feb 20, 2019
Merged

Conversation

@jklukas
Copy link
Contributor

jklukas commented Feb 15, 2019

This change ensures that the core module javadoc:

./gradlew ':beam-sdks-java-core:javadoc'

no longer produces the string @literal anywhere in the produced html. There may still be some instances in other modules.

Many code examples in the core SDK use {@literal @} inside of a {@code} block, which does not escape the @ as expected, but rather produces the text {@literal @}.

There's unfortunately not any single solution that's elegant in all cases. The SDK already uses a variety of markup styles for code examples, and I expect that to continue to be the case depending on the complexity of the example.

I've made additional whitespace and typo fixes along with changing how @literal is used such that each of these code examples now renders nicely with the current Java 8 build.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@jklukas jklukas force-pushed the jklukas:javadoc-at-literals branch from 0e62a1f to 2135147 Feb 15, 2019
@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 15, 2019

R: @echauchot

This is a follow-up to #7852.

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 15, 2019

Run Java PreCommit

Copy link
Contributor

echauchot left a comment

Thx ! Some strange @literal in at least 2 places, also please fix checkstyle errors

* public Accum addInput(Accum accum, Integer input) {
* accum.sum += input;
* accum.count++;
* return accum;
* }
* public Accum mergeAccumulators(Iterable<Accum> accums) {
*
* public Accum{@literal mergeAccumulators(Iterable<Accum> accums)} {

This comment has been minimized.

Copy link
@echauchot

echauchot Feb 18, 2019

Contributor

wrong I guess ?

This comment has been minimized.

Copy link
@jklukas

jklukas Feb 18, 2019

Author Contributor

In this particular case, we're using <pre><code> for the overall block, which doesn't handle <>. The @literal added here is to accomodate the brackets in <Accum>.

This comment has been minimized.

Copy link
@echauchot

echauchot Feb 18, 2019

Contributor

Fair enough as it is rendered well

* public Double extractOutput(Accum accum) {
* return ((double) accum.sum) / accum.count;
* }
* }
* }{@literal

This comment has been minimized.

Copy link
@echauchot

echauchot Feb 18, 2019

Contributor

wrong ?

This comment has been minimized.

Copy link
@jklukas

jklukas Feb 18, 2019

Author Contributor

Same here, the @literal is to handle the angle brackets on the next two lines. We could use @code in these two cases instead, but it's redundant with the <code> wrapping this entire example.

This comment has been minimized.

Copy link
@echauchot

echauchot Feb 18, 2019

Contributor

ditto

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 18, 2019

Run Java PreCommit

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 18, 2019

Does Jenkins store rendered Javadoc somewhere? I imagine reviewing this would be easier if you had the rendered results to cross-check. My process for creating this PR was to run the gradle javadoc task after changing each class to iterate on getting the whitespace and formatting correct, but I understand if you want to hold off on merging until you have a better basis for validating the changes.

@echauchot

This comment has been minimized.

Copy link
Contributor

echauchot commented Feb 18, 2019

@jklukas As they are only minor changes I just went through the changes visually and stopped on things that looked odd to me, then I checked out the branch and rendered that javadoc. It renders correctly on these spots. So LGTM, modulo you fix checkstyle

@jklukas jklukas force-pushed the jklukas:javadoc-at-literals branch from 2135147 to 97d1d9c Feb 18, 2019
@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 18, 2019

Run Java_Examples_Dataflow PreCommit

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 18, 2019

@echauchot - Apologies for missing the checkstyle errors. Added a new commit to address those and the java precommit is now green. I also rebased on master, and I'm assuming the Java_Examples_Dataflow failure is either transient or broken on master.

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 18, 2019

Run Java_Examples_Dataflow PreCommit

Copy link
Contributor

echauchot left a comment

LGTM, please squash the second commit into the first one

This change ensures that the core module javadoc:

    ./gradlew ':beam-sdks-java-core:javadoc'

no longer produces the string `@literal` anywhere in the produced html.
There may still be some instances in other modules.

Many code examples in the core SDK use `{@literal @}` inside of a `{@code}`
block, which does not escape the `@` as expected, but rather produces the text
`{@literal @}`.

There's unfortunately not any single solution that's elegant in all cases.
The SDK already uses a variety of markup styles for code examples, and I expect
that to continue to be the case depending on the complexity of the example.

I've made additional whitespace fixes along with changing how `@literal` is used
such that each of these code examples now renders nicely with the current
Java 8 build.
@jklukas jklukas force-pushed the jklukas:javadoc-at-literals branch from 97d1d9c to 0be65a2 Feb 19, 2019
@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 19, 2019

Run Java PreCommit

@jklukas

This comment has been minimized.

Copy link
Contributor Author

jklukas commented Feb 19, 2019

@echauchot - Squashed to a single commit. Java PreCommit is failing due to an unrelated GrpcDataServiceTest result.

Copy link
Contributor

echauchot left a comment

LGTM, failure is due to an unrelated flaky test.

@echauchot echauchot merged commit bf9961c into apache:master Feb 20, 2019
4 of 5 checks passed
4 of 5 checks passed
Java ("Run Java PreCommit") FAILURE
Details
JavaPortabilityApi ("Run JavaPortabilityApi PreCommit") SUCCESS
Details
Java_Examples_Dataflow ("Run Java_Examples_Dataflow PreCommit") SUCCESS
Details
RAT ("Run RAT PreCommit") SUCCESS
Details
Spotless ("Run Spotless PreCommit") SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.