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

fix: format proto comments in Client Overview #2280

Merged
merged 4 commits into from Dec 11, 2023
Merged

Conversation

alicejli
Copy link
Contributor

@alicejli alicejli commented Dec 1, 2023

Follow up PR to #2114 which breaks generation for java-bigqueryreservation due to errant formatting of the proto comments.

cl/587006892 contains the mock for bigqueryreservation's ReservationServiceClent.

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

@alicejli alicejli requested a review from a team as a code owner December 1, 2023 15:52
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 1, 2023
@@ -242,7 +242,7 @@ private static String createTableOfMethods(List<MethodAndVariants> methodAndVari
.append(method.method)
.append("</td>\n")
.append(" <td>")
.append("<p>" + method.description + "</p>")
.append(CommentFormatter.formatAsJavaDocComment(method.description, null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make the whole table a String and format with CommentFormatter? This fix should be good for now, but in the future, someone else may add another line with dynamic content that could result in the same bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The challenge with formatting the whole table with CommentFormatter is that it replaces all of the html elements for the table with the HTML-escaped character sequence for them, so the output doesn't stay formatted as a table. You can see the example output here: cl/572240700

@blakeli0
Copy link
Collaborator

blakeli0 commented Dec 1, 2023

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

Can we mock this scenario with showcase? By adding the problematic comment to one of the showcase protos.

@alicejli
Copy link
Contributor Author

alicejli commented Dec 5, 2023

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

Can we mock this scenario with showcase? By adding the problematic comment to one of the showcase protos.

Good idea - PR opened: googleapis/gapic-showcase#1391

Copy link

sonarcloud bot commented Dec 7, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link

sonarcloud bot commented Dec 7, 2023

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alicejli
Copy link
Contributor Author

alicejli commented Dec 7, 2023

@blakeli0 I added a failing test for JavaWriter per our offline discussion. Let me know any additional thoughts, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants