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

chore: add special characters to testing.proto comments #1391

Merged
merged 2 commits into from Dec 11, 2023

Conversation

alicejli
Copy link
Contributor

@alicejli alicejli commented Dec 5, 2023

Adding some proto comments to both a service and RPC that contain special characters. This is to help catch generator formatting issues. (e.g. googleapis/sdk-platform-java#2280)

@alicejli alicejli requested review from a team as code owners December 5, 2023 16:42
@burkedavison
Copy link
Contributor

LGTM, but would like to have a showcase-go team member review.

@lqiu96
Copy link
Contributor

lqiu96 commented Dec 5, 2023

QQ: Is there a reason why this is added as a showcase change instead of a unit test/ integration test inside the Java Generator? I think this could also been caught with a test to the CommentFormatter.

I'm not opposed since updating here could also help other languages catch the same issue.

@blakeli0
Copy link
Contributor

blakeli0 commented Dec 5, 2023

QQ: Is there a reason why this is added as a showcase change instead of a unit test/ integration test inside the Java Generator? I think this could also been caught with a test to the CommentFormatter.

I'm not opposed since updating here could also help other languages catch the same issue.

I agree, I think we should test it in both unit tests and showcase tests, for the exact same reason you mentioned. Not integration tests though as it is dependent on the actual service protos which we don't control.

@alicejli
Copy link
Contributor Author

alicejli commented Dec 7, 2023

QQ: Is there a reason why this is added as a showcase change instead of a unit test/ integration test inside the Java Generator? I think this could also been caught with a test to the CommentFormatter.
I'm not opposed since updating here could also help other languages catch the same issue.

I agree, I think we should test it in both unit tests and showcase tests, for the exact same reason you mentioned. Not integration tests though as it is dependent on the actual service protos which we don't control.

I will look to add a test in googleapis/sdk-platform-java#2280. In the meantime, @noahdietz / @vchudnov-g ; let us know if you have any concerns about the addition of these comments to the showcase test. Thanks!

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

I personally don't have a problem with this. We can find out if some of the more markdown-sensitive languages (Python, C# I think) will dislike this though.

I'd say go ahead and we can find out!

@alicejli alicejli merged commit 1d6dde2 into main Dec 11, 2023
11 checks passed
@alicejli alicejli deleted the addCommentsToTestFormatting branch December 11, 2023 18:49
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

5 participants