feat: add support for custom codecs in gRPC server configuration #26
Conversation
PR SummaryAdds
Written by Cursor Bugbot for commit f629919. This will update automatically on new commits. Configure here. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new --codecs CLI flag and threads parsed codec module names through the Elixir code generator; updates generation and README to emit Changes
Sequence DiagramsequenceDiagram
participant CLI as User/CLI
participant Parser as Flag Parser
participant Gen as Code Generator
CLI->>Parser: receive --codecs="Mod.A, Mod.B"
Parser->>Parser: parseCodecs -> split, trim
Parser-->>Gen: []string{"Mod.A","Mod.B"}
Gen->>Gen: generateElixirFile(..., codecs)
Gen->>Gen: generateServiceModule emits server opts (http_transcode?, handler_module_prefix?, codecs?)
Gen-->>CLI: output generated Elixir files with codecs in GRPC.Server options
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yordis
force-pushed
the
yordis/feat-server-opts-1
branch
from
3230f28 to
725cd54
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
♻️ Duplicate comments (1)
cmd/protoc-gen-elixir-grpc/main.go (1)
78-123: Critical: Trailing comma bug loses the last key-value pair.This issue was previously identified and remains unresolved. When a parameter string ends with a trailing comma (e.g.,
"http_transcode=true,"), the last valid key-value pair is lost because the empty final segment triggerscontinueat line 92, skipping the save logic at lines 117-118.This affects all plugin parameters and could silently drop user configuration. For example:
- Input:
"http_transcode=true,"→ Result: empty map (http_transcode setting lost)- Input:
"codecs=GRPC.Codec.Proto,"→ Result: empty map (codecs setting lost)Apply this fix to save the key-value pair before processing the next segment:
func parseKeyValuePairs(paramStr string) map[string]string { result := make(map[string]string) segments := strings.Split(paramStr, ",") var currentKey string var currentValue strings.Builder for i, segment := range segments { segment = strings.TrimSpace(segment) - if segment == "" { - continue - } // Check if this segment contains an '=' sign if idx := strings.Index(segment, "="); idx > 0 { // This is a new key=value pair // Save the previous key-value if exists if currentKey != "" { result[currentKey] = currentValue.String() } // Start new key-value currentKey = strings.TrimSpace(segment[:idx]) valueStart := strings.TrimSpace(segment[idx+1:]) currentValue.Reset() currentValue.WriteString(valueStart) - } else { + } else if segment != "" { // This is a continuation of the current value if currentKey != "" { currentValue.WriteString(",") currentValue.WriteString(segment) } } - - // If this is the last segment, save the current key-value - if i == len(segments)-1 && currentKey != "" { - result[currentKey] = currentValue.String() - } } + // Save the final key-value pair + if currentKey != "" { + result[currentKey] = currentValue.String() + } + return result }
🧹 Nitpick comments (2)
cmd/protoc-gen-elixir-grpc/main_test.go (1)
778-822: LGTM! Good coverage forparseCodecs.The table-driven tests cover important edge cases including spaces and empty items.
However, consider adding tests for the
parseKeyValuePairsfunction, especially the edge case where parameter strings have trailing commas (e.g.,"key1=value1,key2=value2,"), which relates to the existing bug flagged in the past review.Do you want me to generate test cases for
parseKeyValuePairs?cmd/protoc-gen-elixir-grpc/main.go (1)
59-59: Consider multi-line formatting for improved readability.The usage string is quite long and could be difficult to read. Consider breaking it into multiple concatenated lines for better maintainability.
Example:
- usage = "\n\nFlags:\n -h, --help\tPrint this help and exit.\n --version\tPrint the version and exit.\n --handler_module_prefix\tCustom Elixir module prefix for handler modules instead of protobuf package.\n --http_transcode\tEnable HTTP transcoding support (adds http_transcode: true to use GRPC.Server).\n --codecs\tComma-separated list of codec modules (e.g., 'GRPC.Codec.Proto,GRPC.Codec.WebText,GRPC.Codec.JSON')." + usage = "\n\nFlags:\n" + + " -h, --help\tPrint this help and exit.\n" + + " --version\tPrint the version and exit.\n" + + " --handler_module_prefix\tCustom Elixir module prefix for handler modules instead of protobuf package.\n" + + " --http_transcode\tEnable HTTP transcoding support (adds http_transcode: true to use GRPC.Server).\n" + + " --codecs\tComma-separated list of codec modules (e.g., 'GRPC.Codec.Proto,GRPC.Codec.WebText,GRPC.Codec.JSON')."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/protoc-gen-elixir-grpc/README.md(3 hunks)cmd/protoc-gen-elixir-grpc/main.go(7 hunks)cmd/protoc-gen-elixir-grpc/main_test.go(9 hunks)
🔇 Additional comments (7)
cmd/protoc-gen-elixir-grpc/README.md (2)
84-109: LGTM! Clear documentation for custom codecs.The documentation clearly explains the codecs feature with a practical example showing both configuration and generated output.
125-125: LGTM! Consistent with the new feature.The updated example properly demonstrates combining the new codecs option with existing options.
cmd/protoc-gen-elixir-grpc/main_test.go (2)
113-114: LGTM! Improved formatting for GRPC.Server options.The multi-line format improves readability and is consistent with Elixir formatting conventions, especially when multiple options are present.
387-545: LGTM! Comprehensive test coverage for codecs feature.The tests cover key scenarios including basic usage, space handling, and combination with other options. The expected output correctly matches the documented format.
cmd/protoc-gen-elixir-grpc/main.go (3)
125-140: LGTM! Clean implementation of codec parsing.The function correctly handles empty strings, spaces, and filters empty items. The logic is straightforward and aligns well with the test coverage.
176-180: LGTM! Clean integration of codecs through the generation pipeline.The codecs parameter is properly threaded through the generation flow, following the same pattern as existing options.
Also applies to: 216-216, 234-234, 249-249, 263-263
278-292: LGTM! Correct generation of codecs option.The multi-line format improves readability, and the conditional rendering of options is implemented correctly. The codecs list is properly formatted as an Elixir list with comma-separated codec module names.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-server-opts-1
branch
from
725cd54 to
f629919
Compare
No description provided.