fix: check r.err != nil but return a nil value error err
#1028
Conversation
Signed-off-by: alingse <[email protected]>
Signed-off-by: alingse <[email protected]>
📝 WalkthroughWalkthroughThe pull request introduces modifications to the error handling logic in the Changes
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![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
🧹 Nitpick comments (1)
zstd/snappy.go (1)
Line range hint
200-244: Consider standardizing error handling patternsThe changes improve error handling consistency in this file. Consider:
- Documenting this error handling pattern in the project's coding guidelines
- Adding error handling consistency checks to the CI pipeline
- Using the mentioned linter to detect similar issues across the codebase
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zstd/snappy.go(2 hunks)
🔇 Additional comments (2)
zstd/snappy.go (2)
242-244: LGTM: Consistent error handling
The change maintains consistent error handling with the compressed data path, correctly propagating r.err from the write operation.
Let's verify the error handling consistency across all code paths:
✅ Verification successful
Error handling is consistently using r.err throughout the file
The verification confirms that the change is correct and consistent with the error handling pattern throughout the file. The analysis shows:
- All error returns in the file consistently use
r.errfor propagating errors from write operations - The only cases where local
errvariables are used are for temporary error handling from internal operations (likeencodeLits,snappyDecodedLen, etc.) which are then properly assigned tor.err - The pattern of
n, r.err = w.Write()followed byreturn written, r.erris used consistently across all write operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all error returns in the Convert method to ensure consistency
ast-grep --pattern 'return $_, $err'
Length of output: 38
Script:
#!/bin/bash
# Search for return statements with error handling in the file
rg -U "return.*err" zstd/snappy.go
Length of output: 789
Script:
#!/bin/bash
# Get more context around error handling in the file
rg -B 5 -A 5 "r\.err" zstd/snappy.go
Length of output: 5434
Script:
#!/bin/bash
# Look for error variable declarations and assignments
ast-grep --pattern 'var err = $_' zstd/snappy.go
rg "err :?=" zstd/snappy.go
Length of output: 1141
200-202: LGTM: Correct error propagation
The change properly returns r.err instead of a potentially nil error, ensuring accurate error state propagation from the write operation.
Let's verify error handling in similar patterns:
✅ Verification successful
Error handling pattern is consistent with codebase practices
The search results confirm that returning a struct's error field (e.g., r.err, z.err) after write operations is a common and established pattern throughout the codebase. This is evident in multiple packages including zstd, gzip, zlib, and s2. The change in zstd/snappy.go aligns with this consistent error handling approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns in write operations
rg -U "(\w+), (\w+\.)err :=.*Write.*\n.*return .*, \2err" --type go
Length of output: 330
Script:
#!/bin/bash
# Search for similar error handling patterns in write operations using PCRE2
rg -U --pcre2 "(\w+), (\w+\.)err :=.*Write.*\n.*return .*, \2err" --type go
# Let's also try a simpler pattern to catch error handling after Write operations
rg -A 2 ".*Write.*\n.*return.*err" --type go
Length of output: 316
Script:
#!/bin/bash
# Let's search for Write operations and their error handling patterns
rg -U "Write.*\n.*return.*err" --type go
# Also search for the specific pattern of error field access
ast-grep --pattern 'return $_, $_.err'
Length of output: 8750
|
|
IMO, maybe the code should flow one style
btw, I create a linter to detect code that returns a non-relevant nilness error bug. I checked the top 1000 GitHub Go repositories and found this, all result listed in alingse/sundrylint#4
Summary by CodeRabbit