Repository navigation
feat: packet - #89
Merged
Merged
feat: packet#89
Conversation
📝 WalkthroughWalkthroughThis PR adds Mermaid packet diagram support to the library through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Code Metrics Report
Details | | main (f2aca2e) | #89 (0cffbc9) | +/- |
|---------------------|----------------|---------------|-------|
+ | Coverage | 90.2% | 90.4% | +0.1% |
| Files | 42 | 44 | +2 |
| Lines | 1808 | 1888 | +80 |
+ | Covered | 1632 | 1707 | +75 |
- | Test Execution Time | 4s | 5s | +1s |Code coverage of files in pull request scope (0.0% → 93.7%)
Reported by octocov |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mermaid/packet/packet_test.go (2)
19-65: Allwantstrings using"packet"as the opening keyword are downstream of the bug inpacket.go— they'll need updating to"packet-beta"when the root cause is fixed. See thepacket.gocomment for details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mermaid/packet/packet_test.go` around lines 19 - 65, The tests in TestNewDiagram use "packet" as the diagram opening keyword but the implementation will be changed to "packet-beta"; update expected strings in TestNewDiagram (the want fields in the test cases) to use "packet-beta" (and corresponding multi-line expectations such as the title case) so they match NewDiagram/diagram.String() output; verify the third case still expects an error when title contains newline and leave wantErr unchanged.
67-95: Same downstream keyword issue —packetinwantshould bepacket-beta.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mermaid/packet/packet_test.go` around lines 67 - 95, The test TestDiagram_Build expects the old top-level keyword; update the expected output in the want string to use "packet-beta" instead of "packet". Locate the want variable in TestDiagram_Build (within packet_test.go) and replace the leading line `packet` with `packet-beta` so the expected output matches what NewDiagram/Build now emits.
🧹 Nitpick comments (2)
mermaid/packet/packet.go (1)
180-194:normalizeQuotedcallsstrings.TrimSpaceredundantly in the validated call path.
quoteis only ever called fromFieldandNext, both of which first pass the label throughvalidateLabel— which already trims whitespace. TheTrimSpaceinsidenormalizeQuotedis a no-op at runtime for those callers (and the test that callsquotedirectly passes a non-whitespace-padded value). The outer-quote detection logic is still useful; only the redundant trim is worth noting.♻️ Optional refactor
func normalizeQuoted(value string) string { - trimmed := strings.TrimSpace(value) - if len(trimmed) >= 2 && strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) { - return trimmed[1 : len(trimmed)-1] + if len(value) >= 2 && strings.HasPrefix(value, `"`) && strings.HasSuffix(value, `"`) { + return value[1 : len(value)-1] } - return trimmed + return value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mermaid/packet/packet.go` around lines 180 - 194, normalizeQuoted currently calls strings.TrimSpace redundantly because callers (Field and Next via validateLabel) already trim labels; remove the TrimSpace to avoid double-trimming but keep the outer-quote stripping logic intact. In practice, change normalizeQuoted to operate on the raw value (no TrimSpace) and only strip surrounding double quotes if present, and ensure quote still calls normalizeQuoted; verify callers relying on validateLabel still trim input.mermaid/packet/packet_test.go (1)
233-248:errors.Is(d.Error(), err)— sound, but slightly over-specified.
Build()stores the error ind.errand then returns it, sod.Error()anderrare the same pointer;d.Error() == errwould suffice.errors.Isworks here (it trivially matches identical pointers) and is more future-proof if the implementation ever wraps, so this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mermaid/packet/packet_test.go` around lines 233 - 248, The test TestDiagram_BuildStoresError over-specifies the comparison by using errors.Is(d.Error(), err); change it to a direct pointer equality check (d.Error() == err) since Build() stores the same error in d.err and returns it; update the assertion in TestDiagram_BuildStoresError to use == to reflect that Error() and the returned err are identical (referencing NewDiagram, Build, and Error methods).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/packet/main.go`:
- Around line 29-35: The chained call to
packet.NewDiagram(...).Next(...).Field(...).String() hides any error state set
during Next/Field; split the chain into a named variable (e.g., diag :=
packet.NewDiagram(...).Next(...).Field(...)), then call the terminal method and
check its error return (e.g., s, err := diag.String(); if err != nil {
processLogger.Error(...); os.Exit(1) }) before writing to generated.md so
malformed diagrams are not silently emitted; reference the constructors/mutators
NewDiagram, Next, Field and the terminal String in your changes.
---
Duplicate comments:
In `@mermaid/packet/packet_test.go`:
- Around line 19-65: The tests in TestNewDiagram use "packet" as the diagram
opening keyword but the implementation will be changed to "packet-beta"; update
expected strings in TestNewDiagram (the want fields in the test cases) to use
"packet-beta" (and corresponding multi-line expectations such as the title case)
so they match NewDiagram/diagram.String() output; verify the third case still
expects an error when title contains newline and leave wantErr unchanged.
- Around line 67-95: The test TestDiagram_Build expects the old top-level
keyword; update the expected output in the want string to use "packet-beta"
instead of "packet". Locate the want variable in TestDiagram_Build (within
packet_test.go) and replace the leading line `packet` with `packet-beta` so the
expected output matches what NewDiagram/Build now emits.
---
Nitpick comments:
In `@mermaid/packet/packet_test.go`:
- Around line 233-248: The test TestDiagram_BuildStoresError over-specifies the
comparison by using errors.Is(d.Error(), err); change it to a direct pointer
equality check (d.Error() == err) since Build() stores the same error in d.err
and returns it; update the assertion in TestDiagram_BuildStoresError to use ==
to reflect that Error() and the returned err are identical (referencing
NewDiagram, Build, and Error methods).
In `@mermaid/packet/packet.go`:
- Around line 180-194: normalizeQuoted currently calls strings.TrimSpace
redundantly because callers (Field and Next via validateLabel) already trim
labels; remove the TrimSpace to avoid double-trimming but keep the outer-quote
stripping logic intact. In practice, change normalizeQuoted to operate on the
raw value (no TrimSpace) and only strip surrounding double quotes if present,
and ensure quote still calls normalizeQuoted; verify callers relying on
validateLabel still trim input.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.mddoc/es/README.mddoc/fr/README.mddoc/ja/README.mddoc/ko/README.mddoc/packet/generated.mddoc/packet/main.godoc/ru/README.mddoc/zh-cn/README.mdmermaid/packet/config.gomermaid/packet/examples_test.gomermaid/packet/packet.gomermaid/packet/packet_test.go
Comment on lines
+29
to
+35
| diagram := packet.NewDiagram(io.Discard, packet.WithTitle("UDP Packet")). | ||
| Next(16, "Source Port"). //nolint:mnd | ||
| Next(16, "Destination Port"). //nolint:mnd | ||
| Field(32, 47, "Length"). //nolint:mnd | ||
| Field(48, 63, "Checksum"). //nolint:mnd | ||
| Field(64, 95, "Data (variable length)"). //nolint:mnd | ||
| String() |
There was a problem hiding this comment.
Diagram error state is silently discarded by chaining straight to .String().
If any field call in the chain sets an error (e.g., after a future refactor changes the hardcoded values), String() still returns a partial body with no indication of failure, and the malformed content gets written to generated.md without any warning.
Split the chain so the error can be checked:
🛡️ Proposed fix
- diagram := packet.NewDiagram(io.Discard, packet.WithTitle("UDP Packet")).
- Next(16, "Source Port"). //nolint:mnd
- Next(16, "Destination Port"). //nolint:mnd
- Field(32, 47, "Length"). //nolint:mnd
- Field(48, 63, "Checksum"). //nolint:mnd
- Field(64, 95, "Data (variable length)"). //nolint:mnd
- String()
+ d := packet.NewDiagram(io.Discard, packet.WithTitle("UDP Packet")).
+ Next(16, "Source Port"). //nolint:mnd
+ Next(16, "Destination Port"). //nolint:mnd
+ Field(32, 47, "Length"). //nolint:mnd
+ Field(48, 63, "Checksum"). //nolint:mnd
+ Field(64, 95, "Data (variable length)") //nolint:mnd
+ if d.Error() != nil {
+ panic(d.Error())
+ }
+ diagram := d.String()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| diagram := packet.NewDiagram(io.Discard, packet.WithTitle("UDP Packet")). | |
| Next(16, "Source Port"). //nolint:mnd | |
| Next(16, "Destination Port"). //nolint:mnd | |
| Field(32, 47, "Length"). //nolint:mnd | |
| Field(48, 63, "Checksum"). //nolint:mnd | |
| Field(64, 95, "Data (variable length)"). //nolint:mnd | |
| String() | |
| d := packet.NewDiagram(io.Discard, packet.WithTitle("UDP Packet")). | |
| Next(16, "Source Port"). //nolint:mnd | |
| Next(16, "Destination Port"). //nolint:mnd | |
| Field(32, 47, "Length"). //nolint:mnd | |
| Field(48, 63, "Checksum"). //nolint:mnd | |
| Field(64, 95, "Data (variable length)") //nolint:mnd | |
| if d.Error() != nil { | |
| panic(d.Error()) | |
| } | |
| diagram := d.String() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/packet/main.go` around lines 29 - 35, The chained call to
packet.NewDiagram(...).Next(...).Field(...).String() hides any error state set
during Next/Field; split the chain into a named variable (e.g., diag :=
packet.NewDiagram(...).Next(...).Field(...)), then call the terminal method and
check its error return (e.g., s, err := diag.String(); if err != nil {
processLogger.Error(...); os.Exit(1) }) before writing to generated.md so
malformed diagrams are not silently emitted; reference the constructors/mutators
NewDiagram, Next, Field and the terminal String in your changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation