Fix format of proto files #2778
Conversation
Signed-off-by: vankichi <[email protected]>
📝 WalkthroughWalkthroughThe pull request introduces documentation improvements across multiple proto files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apis/proto/v1/vald/update.proto (1)
Line range hint
138-146: TODO comments need to be addressed in UpdateTimestamp method.The UpdateTimestamp method documentation contains TODO placeholders for Status Code and Troubleshooting sections.
Would you like me to help generate the missing documentation sections for UpdateTimestamp? I can analyze similar methods in the codebase to propose appropriate status codes and troubleshooting guidelines.
apis/proto/v1/vald/remove.proto (1)
89-89: Add missing period at the end of the table row.For consistency with other table entries, add a period at the end of the INTERNAL error description.
- // | INTERNAL | Target Vald cluster or network route has some critical error. | Check target Vald cluster first and check network route including ingress as second. + // | INTERNAL | Target Vald cluster or network route has some critical error. | Check target Vald cluster first and check network route including ingress as second. |apis/proto/v1/vald/search.proto (2)
Line range hint
153-157: Consider enhancing the gRPC size limitation noticeThe warning about gRPC message size limitations is valuable, but could be more specific. Consider adding:
- The default message size limit (4MB for gRPC)
- How to configure custom message size limits
- Guidelines for batching requests
<div class="notice"> gRPC has a message size limitation.<br> -Please be careful that the size of the request exceeds the limit. +Please ensure your request size does not exceed the gRPC message size limit (default: 4MB).<br> +For larger requests, consider:<br> +- Using streaming RPCs instead<br> +- Configuring custom message size limits<br> +- Batching your requests into smaller chunks </div>Also applies to: 189-193
Line range hint
387-387: Fix comment formattingThere's an extra forward slash in the comment line which breaks the formatting consistency.
- // // --- + // ---apis/proto/v1/vald/index.proto (1)
Line range hint
26-63: Consider proto design best practicesWhile the current implementation is functional, consider these proto design recommendations:
Request/Response Naming:
- Following standard proto conventions, consider renaming request/response types to be more specific (e.g.,
IndexDetailRequest/IndexDetailResponse)- This improves API clarity and follows buf's recommended naming patterns
Empty Request Pattern:
- The service uses
Emptyrequests consistently across all methods- Consider whether some operations might benefit from parameterization in the future
- If parameters might be needed later, defining specific request messages now could prevent breaking changes
Would you like assistance in implementing these recommendations? I can help generate the revised proto definitions.
🧰 Tools
🪛 buf (1.47.2)
34-36: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
34-34: RPC request type "Empty" should be named "IndexInfoRequest" or "IndexIndexInfoRequest".
(RPC_REQUEST_STANDARD_NAME)
34-34: RPC response type "Count" should be named "IndexInfoResponse" or "IndexIndexInfoResponse".
(RPC_RESPONSE_STANDARD_NAME)
40-42: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
40-40: RPC request type "Empty" should be named "IndexDetailRequest" or "IndexIndexDetailRequest".
(RPC_REQUEST_STANDARD_NAME)
40-40: RPC response type "Detail" should be named "IndexDetailResponse" or "IndexIndexDetailResponse".
(RPC_RESPONSE_STANDARD_NAME)
46-46: RPC request type "Empty" should be named "IndexStatisticsRequest" or "IndexIndexStatisticsRequest".
(RPC_REQUEST_STANDARD_NAME)
46-46: RPC response type "Statistics" should be named "IndexStatisticsResponse" or "IndexIndexStatisticsResponse".
(RPC_RESPONSE_STANDARD_NAME)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apis/proto/v1/vald/filter.proto(5 hunks)apis/proto/v1/vald/index.proto(1 hunks)apis/proto/v1/vald/insert.proto(4 hunks)apis/proto/v1/vald/object.proto(3 hunks)apis/proto/v1/vald/remove.proto(7 hunks)apis/proto/v1/vald/search.proto(16 hunks)apis/proto/v1/vald/update.proto(4 hunks)apis/proto/v1/vald/upsert.proto(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- apis/proto/v1/vald/object.proto
- apis/proto/v1/vald/upsert.proto
- apis/proto/v1/vald/filter.proto
🧰 Additional context used
🪛 buf (1.47.2)
apis/proto/v1/vald/index.proto
40-42: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
40-40: RPC request type "Empty" should be named "IndexDetailRequest" or "IndexIndexDetailRequest".
(RPC_REQUEST_STANDARD_NAME)
40-40: RPC response type "Detail" should be named "IndexDetailResponse" or "IndexIndexDetailResponse".
(RPC_RESPONSE_STANDARD_NAME)
🔇 Additional comments (6)
apis/proto/v1/vald/insert.proto (1)
Line range hint 93-97: LGTM! Important notice about gRPC message size limitations.
The addition of the notice about gRPC message size limitations is valuable for preventing potential runtime issues.
apis/proto/v1/vald/remove.proto (1)
Line range hint 65-71: LGTM! Clear explanation of TimestampRequest behavior.
The added notice effectively explains the AND condition behavior for multiple timestamps and its use cases.
apis/proto/v1/vald/search.proto (2)
46-48: Documentation improvements look great!
The troubleshooting sections are consistently structured across all methods, with clear and actionable error resolutions. The documentation will help developers effectively handle error scenarios.
Also applies to: 78-80, 111-113, 139-141, 170-172, 206-208, 237-239, 270-272, 303-305, 331-333, 362-364, 398-400
Line range hint 1-413: Overall changes look excellent
The documentation improvements significantly enhance the proto file's usability by:
- Adding comprehensive troubleshooting guides
- Maintaining consistency across all methods
- Including important warnings about message size limitations
- Providing specific, actionable error resolutions
The changes align well with the PR's objective while going beyond simple formatting to add substantial value.
apis/proto/v1/vald/index.proto (2)
37-37: LGTM: Consistent whitespace formatting
The whitespace changes between service method definitions improve readability and maintain consistency throughout the proto file.
Also applies to: 43-43
Line range hint 1-63: Verify consistent formatting across all proto files
Since this PR aims to fix proto file formatting, let's ensure consistency across all proto files.
✅ Verification successful
Proto files show consistent formatting and documentation patterns
After analyzing the proto files across the codebase, I can confirm that:
- All service definitions follow consistent formatting with proper indentation
- RPC methods consistently have documentation comments with "Overview" and "Status Code" sections where applicable
- The spacing between RPC methods is uniform within each service
- The file under review (apis/proto/v1/vald/index.proto) follows the same formatting patterns as other proto files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check formatting consistency across proto files
# Look for inconsistent blank line patterns between service methods
# Find all proto files and check for inconsistent blank line patterns
fd -e proto -x awk '
BEGIN { inconsistent=0 }
/^service/ { in_service=1 }
in_service && /^}/ { in_service=0 }
in_service && /^$/ { blank_lines++ }
in_service && /^ rpc/ {
if (prev_blank != blank_lines && prev_blank != "") {
print FILENAME ":" FNR ": Inconsistent blank lines before RPC method"
inconsistent=1
}
prev_blank=blank_lines
blank_lines=0
}
END { exit inconsistent }
'
Length of output: 422
Script:
#!/bin/bash
# Let's try a different approach to check proto formatting consistency
# First, let's see all proto files and their service definitions
# Find all proto files and show service blocks with their RPC methods
fd -e proto -x awk '
/^service/ {
print "\nFile:", FILENAME;
print "Line:", NR;
in_service=1;
print $0
}
in_service && /^[[:space:]]*rpc/ { print $0 }
in_service && /^}/ {
print $0;
in_service=0
}
'
Length of output: 7156
Script:
#!/bin/bash
# Let's check the actual formatting and spacing in proto files
# We'll look at the content between service definitions and their methods
fd -e proto -x awk '
# Print filename when we start processing a new file
FNR==1 { if (FILENAME != prev_file) { print "\nFile:", FILENAME; prev_file=FILENAME } }
# When we find a service definition, print next 10 lines to see formatting
/^service/ {
print "\nService definition at line", FNR;
for(i=0; i<10; i++) {
getline line;
if (line ~ /^}/) break; # Stop if we hit the end of service
print line
}
}
'
Length of output: 6783
🧰 Tools
🪛 buf (1.47.2)
34-36: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
34-34: RPC request type "Empty" should be named "IndexInfoRequest" or "IndexIndexInfoRequest".
(RPC_REQUEST_STANDARD_NAME)
34-34: RPC response type "Count" should be named "IndexInfoResponse" or "IndexIndexInfoResponse".
(RPC_RESPONSE_STANDARD_NAME)
40-42: "payload.v1.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
40-40: RPC request type "Empty" should be named "IndexDetailRequest" or "IndexIndexDetailRequest".
(RPC_REQUEST_STANDARD_NAME)
40-40: RPC response type "Detail" should be named "IndexDetailResponse" or "IndexIndexDetailResponse".
(RPC_RESPONSE_STANDARD_NAME)
46-46: RPC request type "Empty" should be named "IndexStatisticsRequest" or "IndexIndexStatisticsRequest".
(RPC_REQUEST_STANDARD_NAME)
46-46: RPC response type "Statistics" should be named "IndexStatisticsResponse" or "IndexIndexStatisticsResponse".
(RPC_RESPONSE_STANDARD_NAME)
Deploying vald with
|
| Latest commit: |
1ae76c0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b2572f6e.vald.pages.dev |
| Branch Preview URL: | https://refactor-apis-fix-api-proto.vald.pages.dev |
Signed-off-by: vankichi <[email protected]>
Signed-off-by: vankichi <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Filter,Index,Insert,Object,Remove,Search,Update, andUpsert.Search,Insert,Upsert, andRemoveservices.