Add remove_internal_comments option #3560
Conversation
WalkthroughThe changes introduce a new feature to remove internal comments from the OpenAPI output. This is achieved by adding new command-line options, modifying the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (5)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (8)
- Makefile (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (4)
- examples/internal/proto/examplepb/BUILD.bazel
- examples/internal/proto/examplepb/remove_internal_comment.proto
- internal/descriptor/registry.go
- protoc-gen-openapiv2/internal/genopenapi/template.go
Additional comments (Suppressed): 7
Makefile (2)
118-123: The new
buf generatecommands forignore_comment.protoandremove_internal_comment.protoare added correctly. However, ensure that the corresponding.protofiles and their templates exist at the specified paths.124-126: No issues found in these lines. The existing
buf generatecommand forvisibility_rule_echo_service.protoremains unchanged.protoc-gen-openapiv2/main.go (1)
- 31-37: The new flag
remove_internal_commentsis introduced correctly and the description is clear. Ensure that this flag is documented in the user guide or any relevant documentation.protoc-gen-openapiv2/defs.bzl (4)
60-67: The new command-line options
ignore_commentsandremove_internal_commentshave been added correctly to the list of attributes. Ensure that these options are handled properly in the rest of the codebase.112-123: The handling of the new command-line options
ignore_commentsandremove_internal_commentsis implemented correctly here. The options are added to theargslist if they are set.234-241: The new command-line options
ignore_commentsandremove_internal_commentshave been added correctly to the function call to_proto_gen_openapi_impl. Ensure that these options are handled properly in the function definition.314-331: The new command-line options
ignore_commentsandremove_internal_commentshave been defined correctly as boolean attributes with appropriate default values and documentation. The documentation forremove_internal_commentsincludes a link to the relevant Google AIP, which is helpful.
same-id
force-pushed
the
remove-internal-comment
branch
4 times, most recently
from
1cfba5c
to
1bd8026
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (5)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (8)
- Makefile (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (4)
- Makefile
- examples/internal/proto/examplepb/BUILD.bazel
- internal/descriptor/registry.go
- protoc-gen-openapiv2/internal/genopenapi/template.go
Additional comments (Suppressed): 9
examples/internal/proto/examplepb/remove_internal_comment.proto (4)
1-39: Ensure that the new
removeInternalCommentsfunction is correctly removing comments enclosed in(--and--)from the generated OpenAPI documentation. Also, verify that the command-line flag for this feature is working as expected.9-21: The service
Foo2Servicehas been added. Ensure that it is correctly implemented and integrated with the rest of the system.23-36: The message
Foo2Requesthas been added. Make sure that it is being used correctly wherever required.38-39: The message
Foo2Replyhas been added but it doesn't have any fields. If this is intentional, then it's fine. Otherwise, you might want to add some fields to it.protoc-gen-openapiv2/defs.bzl (4)
60-67: The addition of
ignore_commentsandremove_internal_commentsoptions is consistent with the PR summary. Ensure that these options are properly propagated to all functions that need them.112-123: The code correctly adds the new command-line options to the
protoccompiler arguments based on their values. However, ensure that theprotoc-gen-openapiv2plugin supports these options and behaves as expected when they are set.234-241: The propagation of the new options to the
_proto_gen_openapi_implfunction is correct. Make sure that the function uses these options as intended.314-331: The definitions of the new attributes for the Bazel rule are correct. The default values are appropriate (False), and the documentation is clear and helpful. However, ensure that the link provided in the doc for
remove_internal_commentsis accessible and contains relevant information.protoc-gen-openapiv2/main.go (1)
- 131-136: The
removeInternalCommentsoption is set correctly in the registry. As discussed in the comment chain, ensure that the case where bothignoreCommentsandremoveInternalCommentsare set to true is handled appropriately, withignoreCommentstaking precedence as it ignores all comments, not just internal ones.
|
Also, thanks for being a test subject for the AI code review bot I installed 😁. Let me know what the experience is like as a contributor! |
same-id
force-pushed
the
remove-internal-comment
branch
from
1bd8026
to
3a2728f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files ignored due to filter (5)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (9)
- Makefile (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (5)
- Makefile
- examples/internal/proto/examplepb/BUILD.bazel
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
- protoc-gen-openapiv2/defs.bzl
- protoc-gen-openapiv2/main.go
Additional comments (Suppressed): 7
examples/internal/proto/examplepb/remove_internal_comment.proto (4)
1-39: Ensure that the new protobuf file
remove_internal_comment.protois correctly imported and used in other parts of the codebase where necessary. Also, verify if the new serviceFoo2Serviceand its RPC methodFoo2are properly implemented and called.9-21: The comments within the
(-- --)brackets are intended to be excluded from the OpenAPI output as per the PR summary. Ensure that the implementation of this feature is working as expected by checking the generated OpenAPI documentation.23-36: Similar to the previous comment, ensure that the internal comments in the message definitions
Foo2RequestandFoo2Replyare also being excluded from the OpenAPI output.38-39: This comment should also be excluded from the OpenAPI output. Verify this in the generated documentation.
internal/descriptor/registry.go (2)
81-90: The new fields
ignoreComments,removeInternalComments, andenumsAsIntsare added to theRegistrystruct. Ensure that these fields are properly initialized in the constructor or wherever theRegistrystruct is instantiated.588-596: Getter and setter methods for
removeInternalCommentsare correctly implemented.protoc-gen-openapiv2/internal/genopenapi/template.go (1)
- 2259-2262: Ensure that the
GetRemoveInternalCommentsmethod is working as expected and that the flag is being set correctly in the command-line interface.
same-id
force-pushed
the
remove-internal-comment
branch
from
3a2728f
to
7d99e1b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files ignored due to filter (5)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (10)
- Makefile (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (6)
- Makefile
- examples/internal/proto/examplepb/BUILD.bazel
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
- protoc-gen-openapiv2/defs.bzl
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel
- protoc-gen-openapiv2/main.go
Additional comments (Suppressed): 7
examples/internal/proto/examplepb/remove_internal_comment.proto (4)
1-39: Ensure that the new feature
removeInternalCommentsis working as expected by verifying that all comments enclosed within(--and--)are removed from the OpenAPI output when the flag is set.9-21: The service
Foo2Serviceand its RPC methodFoo2have been defined correctly. Ensure that the HTTP option for the RPC method matches the intended API route.23-36: The message
Foo2Requesthas been defined correctly with fieldsusernameandpassword. Make sure these field names align with the application's user model.38-39: The message
Foo2Replyis currently empty. If this is intentional and you're planning to use it as a placeholder for future data, then it's fine. Otherwise, consider adding necessary fields or remove it if it's not needed.internal/descriptor/registry.go (2)
81-90: The addition of the
removeInternalCommentsfield in theRegistrystruct is a good approach to handle the removal of internal comments from the OpenAPI output. This allows for more granular control over comment handling, as opposed to using the existingignoreCommentsfield which would remove all comments.588-596: The setter and getter methods for
removeInternalCommentsare correctly implemented. They follow the same pattern as other similar methods in theRegistrystruct.protoc-gen-openapiv2/internal/genopenapi/template.go (1)
- 2259-2262: Ensure that the
GetRemoveInternalCommentsmethod is properly checking the flag and that the flag is being set correctly in the command-line arguments.
|
👍
czw., 7 wrz 2023, 22:58 użytkownik Harjot Gill ***@***.***>
napisał:
… ***@***.**** commented on this pull request.
------------------------------
In protoc-gen-openapiv2/internal/genopenapi/template.go
<#3560 (comment)>
:
> +func removeInternalComments(comment string) string {
+ c := []string{}
+ for len(comment) > 0 {
+ open := strings.SplitN(comment, "(--", 2)
+ ex := strings.TrimRight(open[0], " \t")
+ // Trim only one line prior to all spaces
+ switch {
+ case strings.HasSuffix(ex, "\r\n"):
+ ex = strings.TrimSuffix(ex, "\r\n")
+ case strings.HasSuffix(ex, "\n"):
+ ex = strings.TrimSuffix(ex, "\n")
+ }
+ if ex != "" {
+ c = append(c, ex)
+ }
+ if len(open) > 1 {
+ comment = open[1]
+ } else {
+ break
+ }
+ close := strings.SplitN(comment, "--)", 2)
+ if len(close) > 1 {
+ comment = close[1]
+ } else {
+ break
+ }
+ }
+ return strings.Join(c, "")
+}
Thanks for being patient test subjects! :)
—
Reply to this email directly, view it on GitHub
<#3560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AUUANJVMGXZ2R2BOTEB5IUTXZIYPVANCNFSM6AAAAAA4FEMVWA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
same-id
force-pushed
the
remove-internal-comment
branch
from
7d99e1b
to
4eeacf6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files ignored due to filter (5)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (11)
- Makefile (1 hunks)
- docs/docs/mapping/customizing_openapi_output.md (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (5)
- Makefile
- docs/docs/mapping/customizing_openapi_output.md
- examples/internal/proto/examplepb/BUILD.bazel
- protoc-gen-openapiv2/defs.bzl
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel
Additional comments (Suppressed): 13
examples/internal/proto/examplepb/remove_internal_comment.proto (4)
1-39: Ensure that the new
remove_internal_commentsflag is correctly implemented and tested to remove internal comments from the OpenAPI output. This includes verifying that all instances of(--and--)are correctly identified and removed, and that no other parts of the comments are affected.9-21: The service
Foo2Serviceand its RPC methodFoo2have been added. Ensure that these changes are reflected in the rest of the codebase where necessary, including any routing or handler functions.23-36: The message
Foo2Requesthas been added with fieldsusernameandpassword. Make sure that these fields are handled correctly in the rest of the codebase, including any validation, serialization, and deserialization logic.38-39: The message
Foo2Replyhas been added but it doesn't contain any fields. If this is intentional, ensure that the rest of the codebase handles empty responses correctly. If not, consider adding the necessary fields.protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (2)
1-2: The build constraints are updated to Go 1.18. Ensure that the rest of the codebase and your deployment environments support this version.
13-26: This fuzz test is a good addition for testing the
removeInternalCommentsfunction with various input scenarios. It compares the output of the function with the expected output generated by a regex, which is a good way to validate the correctness of the function.internal/descriptor/registry.go (2)
81-90: The addition of the
removeInternalCommentsfield in theRegistrystruct is consistent with the existing structure and follows the same pattern as other fields likeignoreCommentsandenumsAsInts. This change aligns with the goal of the PR to add a feature for removing internal comments from the generated OpenAPI output.585-600: The setter and getter methods for
removeInternalCommentsare correctly implemented. They follow the same pattern as the existing methods for other fields in theRegistrystruct. The naming convention is consistent, and the methods do exactly what they're supposed to do: set and get the value ofremoveInternalComments.protoc-gen-openapiv2/main.go (2)
31-37: The new flags
removeInternalCommentsandenumsAsIntsare introduced correctly. The descriptions are clear and provide enough context for their usage. However, as discussed in the comment chain, it would be beneficial to clarify the interaction betweenignoreCommentsandremoveInternalComments. If both flags are set to true,ignoreCommentsshould take precedence because it ignores all comments, not just internal ones.131-137: The
removeInternalCommentsoption is set correctly in the registry. As mentioned in the comment chain, ensure that the case where bothignoreCommentsandremoveInternalCommentsare set to true is handled appropriately. In such a scenario,ignoreCommentsshould take precedence as it ignores all comments, not just internal ones.protoc-gen-openapiv2/internal/genopenapi/template.go (2)
2259-2263: Ensure that the
GetRemoveInternalCommentsmethod is correctly implemented and tested. Also, verify that theremoveInternalCommentsfunction is called only when necessary to avoid unnecessary computation.2260-2263: The call to
removeInternalCommentsfunction is correctly placed and will remove internal comments if the "remove_internal_comments" flag is set to true.examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go (1)
- 1-171: The generated code looks good overall. It follows the standard conventions for gRPC and HTTP handlers in Go. The error handling is mostly correct, with a few minor issues noted above. The use of context cancellation and metadata propagation is correctly implemented.
| func request_Foo2Service_Foo2_0(ctx context.Context, marshaler runtime.Marshaler, client Foo2ServiceClient, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) { | ||
| var protoReq Foo2Request | ||
| var metadata runtime.ServerMetadata | ||
|
|
||
| newReader, berr := utilities.IOReaderFactory(req.Body) | ||
| if berr != nil { | ||
| return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr) | ||
| } | ||
| if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF { | ||
| return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err) | ||
| } | ||
|
|
||
| msg, err := client.Foo2(ctx, &protoReq, grpc.Header(&metadata.HeaderMD), grpc.Trailer(&metadata.TrailerMD)) | ||
| return msg, metadata, err |
There was a problem hiding this comment.
The error handling in this function could be improved. Currently, if an error occurs when decoding the request body, the function returns immediately with a status error. However, it would be better to close the reader before returning to ensure that resources are properly cleaned up.
newReader, berr := utilities.IOReaderFactory(req.Body)
if berr != nil {
return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
}
if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
+ newReader.Close()
return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
}| func local_request_Foo2Service_Foo2_0(ctx context.Context, marshaler runtime.Marshaler, server Foo2ServiceServer, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) { | ||
| var protoReq Foo2Request | ||
| var metadata runtime.ServerMetadata | ||
|
|
||
| newReader, berr := utilities.IOReaderFactory(req.Body) | ||
| if berr != nil { | ||
| return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr) | ||
| } | ||
| if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF { | ||
| return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err) | ||
| } | ||
|
|
||
| msg, err := server.Foo2(ctx, &protoReq) | ||
| return msg, metadata, err |
There was a problem hiding this comment.
Similar to the previous comment, ensure to close the reader before returning in case of an error during decoding.
newReader, berr := utilities.IOReaderFactory(req.Body)
if berr != nil {
return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
}
if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
+ newReader.Close()
return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
}| conn, err := grpc.DialContext(ctx, endpoint, opts...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| if err != nil { | ||
| if cerr := conn.Close(); cerr != nil { | ||
| grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr) | ||
| } | ||
| return | ||
| } | ||
| go func() { | ||
| <-ctx.Done() | ||
| if cerr := conn.Close(); cerr != nil { | ||
| grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr) | ||
| } | ||
| }() | ||
| }() |
There was a problem hiding this comment.
This function dials a connection and then defers its closure. However, if dialing fails, there's no need to attempt to close the connection. The deferred closure should be moved inside the error check.
conn, err := grpc.DialContext(ctx, endpoint, opts...)
if err != nil {
return err
}
+ defer func() {
+ if cerr := conn.Close(); cerr != nil {
+ grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
+ }
+ }()
- defer func() {
- if err != nil {
- if cerr := conn.Close(); cerr != nil {
- grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
- }
- return
- }
- go func() {
- <-ctx.Done()
- if cerr := conn.Close(); cerr != nil {
- grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
- }
- }()
- }()
Done |
To allow internal comments within the protobuf which won't make it the generated OAS file. Using Google's AIP-192: https://google.aip.dev/192#internal-comments
same-id
force-pushed
the
remove-internal-comment
branch
from
4eeacf6
to
bb30237
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files ignored due to filter (6)
- buf.yaml
- examples/internal/proto/examplepb/remove_internal_comment.buf.gen.yaml
- examples/internal/proto/examplepb/remove_internal_comment.pb.go
- examples/internal/proto/examplepb/remove_internal_comment.pb.gw.go
- examples/internal/proto/examplepb/remove_internal_comment.swagger.json
- examples/internal/proto/examplepb/remove_internal_comment_grpc.pb.go
Files selected for processing (10)
- Makefile (1 hunks)
- docs/docs/mapping/customizing_openapi_output.md (1 hunks)
- examples/internal/proto/examplepb/BUILD.bazel (2 hunks)
- examples/internal/proto/examplepb/remove_internal_comment.proto (1 hunks)
- internal/descriptor/registry.go (2 hunks)
- protoc-gen-openapiv2/defs.bzl (4 hunks)
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel (1 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template.go (2 hunks)
- protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (1 hunks)
- protoc-gen-openapiv2/main.go (2 hunks)
Files skipped from review due to trivial changes (5)
- docs/docs/mapping/customizing_openapi_output.md
- examples/internal/proto/examplepb/BUILD.bazel
- protoc-gen-openapiv2/defs.bzl
- protoc-gen-openapiv2/internal/genopenapi/BUILD.bazel
- protoc-gen-openapiv2/main.go
Additional comments (Suppressed): 10
Makefile (1)
- 118-123: Ensure that the new protobuf files
ignore_comment.protoandremove_internal_comment.protoexist at the specified paths. Also, verify that the corresponding templatesignore_comment.buf.gen.yamlandremove_internal_comment.buf.gen.yamlare correctly set up to generate the desired output.examples/internal/proto/examplepb/remove_internal_comment.proto (4)
1-39: The new proto file
remove_internal_comment.protois well structured and follows the protobuf syntax correctly. The comments that are supposed to be excluded from the OpenAPI output are correctly placed within(-- --)as per Google's AIP-192 for internal comments. Ensure that the new serviceFoo2Serviceand its RPC methodFoo2are properly integrated with the rest of the system.9-21: Ensure that the new service
Foo2Serviceand its RPC methodFoo2are correctly implemented in the server-side code. Also, verify that the HTTP POST route/v1/example/foodoes not conflict with any existing routes.23-36: The message definitions
Foo2RequestandFoo2Replyare correctly defined. However, ensure that these new messages are used correctly wherever required in the codebase.38-39: The
Foo2Replymessage is currently empty. If this is intentional and you plan to use it as a placeholder for future fields, then it's fine. Otherwise, consider adding necessary fields to it.internal/descriptor/registry.go (2)
81-90: The addition of the
removeInternalCommentsfield in theRegistrystruct is consistent with the PR's goal of removing internal comments from the generated OpenAPI output. The comment above the field provides a clear explanation of its purpose and references the relevant AIP for further context.585-600: The setter and getter methods for
removeInternalCommentsare correctly implemented. They follow the same pattern as the existing methods for other fields in theRegistrystruct.protoc-gen-openapiv2/internal/genopenapi/template_fuzz_test.go (2)
1-2: The build constraints are updated to Go 1.18. Ensure that the rest of the codebase and your deployment environment support this version.
13-26: This fuzz test function
FuzzRemoveInternalCommentsis added to test theremoveInternalCommentsfunction. It adds several cases to the fuzzer, then checks that the output ofremoveInternalComments(s)is equal to the result of removing all matches ofreplaceInternalCommentsRegexfroms. If they're not equal, it reports an error. This seems like a good way to ensure thatremoveInternalCommentsbehaves as expected.protoc-gen-openapiv2/internal/genopenapi/template.go (1)
- 2257-2263: The
removeInternalCommentsfunction is correctly used inupdateOpenAPIDataFromComments. It is only called if the "remove_internal_comments" flag is set to true, which is the expected behavior.
| "testing" | ||
| ) | ||
|
|
||
| var replaceInternalCommentsRegex = regexp.MustCompile(`(?s)(\r?\n)?[ \t]*(\(--)((.*?--\))|.*$)?`) |
There was a problem hiding this comment.
This regular expression is used to match internal comments. It seems to correctly identify substrings that start with (-- and end with --), but it also matches any line that starts with (-- and does not have a closing --). This could lead to unexpected behavior if there's a typo in the comment syntax or if the closing tag is missing. Consider adjusting the regex to only match properly closed comments.
There was a problem hiding this comment.
I am not a regex expert, but is this right? It looks to me like it might be since the 3rd capturing group allows --) or .*?
There was a problem hiding this comment.
@johanbrandhorst: You're correct in your observation. The regular expression is designed to match two scenarios:
-
A substring that starts with
(--, ends with--), and may contain any characters (including newlines) in between due to the.*?part. This is the expected format for internal comments. -
A line that starts with
(--but does not have a closing--). In this case, it matches until the end of the string (.*$). This is to handle cases where there might be a typo or missing closing tag.
The | operator in the regex means "or", so it will match either of these two scenarios. However, because of the ? after the second group, the regex engine will try to match the first scenario before falling back to the second one if the first doesn't match. This means it will correctly identify properly closed comments before considering unclosed ones.
So yes, the regular expression is working as intended based on the requirements. However, if you want to strictly enforce the comment syntax and avoid matching lines with missing closing tags, you could remove the |.*$ part from the regex. This would make it only match substrings that start with (-- and end with --).
it's or .*$ Meaning, prefer a closing bracket - but if it wasn't closed, just remove until the end. Oh, how much have I fought with this with the no-lookahead RE2 syntax |
|
Also you can thank the fuzzer test to make sure that the regex is absolutely right :D |
|
Thanks for your contribution! |
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/grpc-ecosystem/grpc-gateway/v2](https://togithub.com/grpc-ecosystem/grpc-gateway) | require | minor | `v2.16.0` -> `v2.18.0` | | [google.golang.org/protobuf](https://togithub.com/protocolbuffers/protobuf-go) | require | minor | `v1.30.0` -> `v1.31.0` | | | | lockFileMaintenance | All locks refreshed | | [jekyll-feed](https://togithub.com/jekyll/jekyll-feed) | | minor | `0.15.1` -> `0.17.0` | --- ### Release Notes <details> <summary>grpc-ecosystem/grpc-gateway (github.com/grpc-ecosystem/grpc-gateway/v2)</summary> ### [`v2.18.0`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.18.0) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.17.1...v2.18.0) #### What's Changed - Enable a few more golangci-lint linters by [@​pkwarren](https://togithub.com/pkwarren) in [grpc-ecosystem/grpc-gateway#3546 - Add .golangci.yml config by [@​pkwarren](https://togithub.com/pkwarren) in [grpc-ecosystem/grpc-gateway#3548 - Fix: path item objects become block scalars \[[#​3557](https://togithub.com/grpc-ecosystem/grpc-gateway/issues/3557)] by [@​qawatake](https://togithub.com/qawatake) in [grpc-ecosystem/grpc-gateway#3566 - Add remove_internal_comments option by [@​same-id](https://togithub.com/same-id) in [grpc-ecosystem/grpc-gateway#3560 #### New Contributors - [@​benjx1990](https://togithub.com/benjx1990) made their first contribution in [grpc-ecosystem/grpc-gateway#3552 - [@​qawatake](https://togithub.com/qawatake) made their first contribution in [grpc-ecosystem/grpc-gateway#3566 **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.17.1...v2.18.0 ### [`v2.17.1`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.17.1) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.17.0...v2.17.1) #### What's Changed - genopenapi: set source code info explicitly by [@​johanbrandhorst](https://togithub.com/johanbrandhorst) in [grpc-ecosystem/grpc-gateway#3544 - Record the filename of files that are missing SourceCodeInfo by [@​ebilling](https://togithub.com/ebilling) in [grpc-ecosystem/grpc-gateway#3545 #### New Contributors - [@​ebilling](https://togithub.com/ebilling) made their first contribution in [grpc-ecosystem/grpc-gateway#3545 **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.17.0...v2.17.1 ### [`v2.17.0`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.17.0) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.16.2...v2.17.0) #### What's Changed - Fix renovate.yml overwriting .bazelrc by [@​adambabik](https://togithub.com/adambabik) in [grpc-ecosystem/grpc-gateway#3451 - fix: successful typo by [@​testwill](https://togithub.com/testwill) in [grpc-ecosystem/grpc-gateway#3510 - Update example buf.gen.yaml by [@​pkwarren](https://togithub.com/pkwarren) in [grpc-ecosystem/grpc-gateway#3522 - fix(deps): replace all uses of golang/protobuf/protobuf by [@​aimuz](https://togithub.com/aimuz) in [grpc-ecosystem/grpc-gateway#3516 - Feature/issue 3051 swagger preserve rpc order by [@​CemGurhan](https://togithub.com/CemGurhan) in [grpc-ecosystem/grpc-gateway#3500 #### New Contributors - [@​testwill](https://togithub.com/testwill) made their first contribution in [grpc-ecosystem/grpc-gateway#3510 - [@​pkwarren](https://togithub.com/pkwarren) made their first contribution in [grpc-ecosystem/grpc-gateway#3522 - [@​aimuz](https://togithub.com/aimuz) made their first contribution in [grpc-ecosystem/grpc-gateway#3516 - [@​CemGurhan](https://togithub.com/CemGurhan) made their first contribution in [grpc-ecosystem/grpc-gateway#3500 **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.16.2...v2.17.0 ### [`v2.16.2`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.16.2) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.16.1...v2.16.2) #### What's Changed - chore: replace the Goreleaser's deprecated option `--rm-dist` to `--clean` by [@​suzuki-shunsuke](https://togithub.com/suzuki-shunsuke) in [grpc-ecosystem/grpc-gateway#3438 - chore: stop using the deprecated field `archives.replacements` by [@​suzuki-shunsuke](https://togithub.com/suzuki-shunsuke) in [grpc-ecosystem/grpc-gateway#3436 #### New Contributors - [@​suzuki-shunsuke](https://togithub.com/suzuki-shunsuke) made their first contribution in [grpc-ecosystem/grpc-gateway#3438 **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.16.1...v2.16.2 ### [`v2.16.1`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.16.1) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.16.0...v2.16.1) #### What's Changed - Rename LICENSE.txt to LICENSE by [@​pgmitche](https://togithub.com/pgmitche) in [grpc-ecosystem/grpc-gateway#3345 - feat: message supports enum field option for query params by [@​zhb127](https://togithub.com/zhb127) in [grpc-ecosystem/grpc-gateway#3352 - remove glog from a bunch of spots by [@​kn100](https://togithub.com/kn100) in [grpc-ecosystem/grpc-gateway#3361 - Improve AIP support in field_configuration.path_param_name containing by [@​same-id](https://togithub.com/same-id) in [grpc-ecosystem/grpc-gateway#3364 - Hide comments on omitted enum default values by [@​same-id](https://togithub.com/same-id) in [grpc-ecosystem/grpc-gateway#3366 - fix(docs): correcting mux routing error code example by [@​tjasko](https://togithub.com/tjasko) in [grpc-ecosystem/grpc-gateway#3409 - Fix the place where format property in placed for repeated fields by [@​far4599](https://togithub.com/far4599) in [grpc-ecosystem/grpc-gateway#3410 - Handle httpbody not having data to return by [@​achew22](https://togithub.com/achew22) in [grpc-ecosystem/grpc-gateway#3415 - bazel: bump Bazel repositories versions by [@​adambabik](https://togithub.com/adambabik) in [grpc-ecosystem/grpc-gateway#3413 #### New Contributors - [@​pgmitche](https://togithub.com/pgmitche) made their first contribution in [grpc-ecosystem/grpc-gateway#3345 - [@​zhb127](https://togithub.com/zhb127) made their first contribution in [grpc-ecosystem/grpc-gateway#3352 - [@​kn100](https://togithub.com/kn100) made their first contribution in [grpc-ecosystem/grpc-gateway#3361 - [@​tjasko](https://togithub.com/tjasko) made their first contribution in [grpc-ecosystem/grpc-gateway#3409 - [@​far4599](https://togithub.com/far4599) made their first contribution in [grpc-ecosystem/grpc-gateway#3410 **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.16.0...v2.16.1 </details> <details> <summary>protocolbuffers/protobuf-go (google.golang.org/protobuf)</summary> ### [`v1.31.0`](https://togithub.com/protocolbuffers/protobuf-go/releases/tag/v1.31.0) [Compare Source](https://togithub.com/protocolbuffers/protobuf-go/compare/v1.30.0...v1.31.0) #### Notable changes <a name="v1.31-notable-changes"></a> **New Features** - [CL/489316](https://go.dev/cl/489316): types/dynamicpb: add NewTypes - Add a function to construct a dynamic type registry from a protoregistry.Files - [CL/489615](https://go.dev/cl/489615): encoding: add MarshalAppend to protojson and prototext **Minor performance improvements** - [CL/491596](https://go.dev/cl/491596): encoding/protodelim: If UnmarshalFrom gets a bufio.Reader, try to reuse its buffer instead of creating a new one - [CL/500695](https://go.dev/cl/500695): proto: store the size of tag to avoid multiple calculations **Bug fixes** - [CL/497935](https://go.dev/cl/497935): internal/order: fix sorting of synthetic oneofs to be deterministic - [CL/505555](https://go.dev/cl/505555): encoding/protodelim: fix handling of io.EOF </details> <details> <summary>jekyll/jekyll-feed (jekyll-feed)</summary> ### [`v0.17.0`](https://togithub.com/jekyll/jekyll-feed/blob/HEAD/History.markdown#0170--2022-10-14) [Compare Source](https://togithub.com/jekyll/jekyll-feed/compare/v0.16.0...v0.17.0) ##### Documentation - Update CI status badge ([#​363](https://togithub.com/jekyll/jekyll-feed/issues/363)) ##### Development Fixes - Add Ruby 3.1 to the CI matrix ([#​365](https://togithub.com/jekyll/jekyll-feed/issues/365)) ##### Minor Enhancements - Allow disabling of jekyll-feed while in development ([#​370](https://togithub.com/jekyll/jekyll-feed/issues/370)) ### [`v0.16.0`](https://togithub.com/jekyll/jekyll-feed/blob/HEAD/History.markdown#0160--2022-01-03) [Compare Source](https://togithub.com/jekyll/jekyll-feed/compare/v0.15.1...v0.16.0) ##### Minor Enhancements - Add support for `page.description` in front matter to become entry `<summary>` ([#​297](https://togithub.com/jekyll/jekyll-feed/issues/297)) ##### Bug Fixes - Fold private methods into the `:render` method as local variables ([#​327](https://togithub.com/jekyll/jekyll-feed/issues/327)) - Check `post.categories` instead of `post.category` ([#​357](https://togithub.com/jekyll/jekyll-feed/issues/357)) - Switched xml_escape for `<![CDATA[]]>` for post content ([#​332](https://togithub.com/jekyll/jekyll-feed/issues/332)) ##### Development Fixes - Add Ruby 3.0 to CI ([#​337](https://togithub.com/jekyll/jekyll-feed/issues/337)) - Lock RuboCop to v1.18.x ([#​348](https://togithub.com/jekyll/jekyll-feed/issues/348)) - Add workflow to release gem via GH Action ([#​355](https://togithub.com/jekyll/jekyll-feed/issues/355)) ##### Documentation - Use `.atom` extension in documented examples since we write an Atom feed ([#​359](https://togithub.com/jekyll/jekyll-feed/issues/359)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on wednesday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/google/osv.dev). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuODMuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
](https://renovatebot.com){kind=link}
- Enable a few more golangci-lint linters [#3546](grpc-ecosystem/grpc-gateway#3546) - Add .golangci.yml config [#3548](grpc-ecosystem/grpc-gateway#3548) - Fix: path item objects become block scalars [#3566](grpc-ecosystem/grpc-gateway#3566) - Add remove_internal_comments option [#3560](grpc-ecosystem/grpc-gateway#3560)
- Enable a few more golangci-lint linters [#3546](grpc-ecosystem/grpc-gateway#3546) - Add .golangci.yml config [#3548](grpc-ecosystem/grpc-gateway#3548) - Fix: path item objects become block scalars [#3566](grpc-ecosystem/grpc-gateway#3566) - Add remove_internal_comments option [#3560](grpc-ecosystem/grpc-gateway#3560)
- Enable a few more golangci-lint linters [#3546](grpc-ecosystem/grpc-gateway#3546) - Add .golangci.yml config [#3548](grpc-ecosystem/grpc-gateway#3548) - Fix: path item objects become block scalars [#3566](grpc-ecosystem/grpc-gateway#3566) - Add remove_internal_comments option [#3560](grpc-ecosystem/grpc-gateway#3560)
- Enable a few more golangci-lint linters [#3546](grpc-ecosystem/grpc-gateway#3546) - Add .golangci.yml config [#3548](grpc-ecosystem/grpc-gateway#3548) - Fix: path item objects become block scalars [#3566](grpc-ecosystem/grpc-gateway#3566) - Add remove_internal_comments option [#3560](grpc-ecosystem/grpc-gateway#3560)
To allow internal comments within the protobuf which won't make it the generated OAS file.
Using Google's AIP-192:
https://google.aip.dev/192#internal-comments
References to other Issues or PRs
Have you read the Contributing Guidelines?
Brief description of what is fixed or changed
Other comments
Summary by CodeRabbit
remove_internal_commentswhich, when set, removes all substrings in comments that start with(--and end with--). This feature aligns with AIP 192: Internal comments.buf generatecommand to the Makefile for generating code based on a template and a protobuf file.removeInternalCommentsfunction to ensure its correctness.remove_internal_commentsoption.