improve parse functions - error handling #137
Conversation
error instead of boolean return
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
## Walkthrough
The parsing functions for integers and floating-point numbers were updated to return detailed error values instead of boolean success flags. All related helper functions and their corresponding tests and benchmarks were modified to use and check for errors, aligning the API with Go's standard error-handling conventions.
## Changes
| Files | Change Summary |
|----------------|------------------------------------------------------------------------------------------------|
| parse.go | Updated all parsing functions to return `error` values instead of boolean flags; adjusted internal logic and added `strconv` import for error types. |
| parse_test.go | Modified all tests and benchmarks to check for errors instead of boolean success flags. |
| README.md | Updated benchmark results reflecting new parsing function implementations and performance metrics.|
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Caller
participant Parser
participant strconv
Caller->>Parser: ParseInt(s)
Parser->>Parser: Validate input
alt Invalid syntax or overflow
Parser->>strconv: Create NumError
Parser->>Caller: return (0, error)
else Valid input
Parser->>Caller: return (value, nil)
endPossibly related PRs
Suggested labels
Suggested reviewers
Poem
Learnt from: ReneWerner87 Learnt from: ReneWerner87 Learnt from: luk3skyw4lker Learnt from: luk3skyw4lker Learnt from: sixcolors Learnt from: sixcolors Learnt from: luk3skyw4lker Learnt from: luk3skyw4lker Learnt from: ReneWerner87 Learnt from: ReneWerner87 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly improves the error handling of the parsing utility functions by migrating from a boolean success/failure return to a more idiomatic Go error return. This change enhances the robustness and debuggability of the parsing logic by providing specific error details, aligning the codebase with standard Go practices.
Highlights
- Error Handling Refactor: All public and internal parsing functions (e.g.,
ParseUint,ParseInt,ParseFloat64,parseDigits,parseSigned, etc.) have been refactored to return a standard Goerrortype instead of a boolean success indicator. This provides more detailed information about parsing failures. - Standard Error Types Adoption: The refactored parsing functions now utilize specific error types from the
strconvpackage, such asstrconv.ErrSyntaxfor invalid input format andstrconv.ErrRangefor overflow/underflow, wrapped withinstrconv.NumErrorfor richer context. - Test Suite Updates: The corresponding test suite (
parse_test.go) has been updated across all parsing functions to assert on the returnederrorvalue, ensuring the new error handling mechanism functions as expected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #137 +/- ##
==========================================
+ Coverage 97.11% 97.12% +0.01%
==========================================
Files 10 10
Lines 727 730 +3
==========================================
+ Hits 706 709 +3
Misses 14 14
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
parse.go(10 hunks)parse_test.go(23 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
PR: gofiber/fiber#2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-07-01T15:49:51.308Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
parse_test.go (12)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/datasources/database/db_mock.go:13-19
Timestamp: 2024-11-23T19:50:06.387Z
Learning: In test code within `clean-code/app/datasources/database/db_mock.go`, adding safety checks like context validation, safe type assertions, and extra documentation is not necessary.
Learnt from: gaby
PR: gofiber/fiber#3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in `ctx_test.go`, it is acceptable to use invalid CIDR notation such as `"0.0.0.1/31junk"` for testing purposes.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/server/server.go:14-16
Timestamp: 2024-11-23T19:50:41.765Z
Learning: In the `clean-code` example at `clean-code/app/server/server.go` using the Go Fiber framework, it's acceptable to omit production-level features like context usage and graceful shutdown handling to keep the example simple.
Learnt from: juls0730
PR: gofiber/recipes#2710
File: tableflip/main.go:61-62
Timestamp: 2024-12-01T01:15:48.126Z
Learning: In the GoFiber `tableflip` recipe (`tableflip/main.go`), the implementation matches the upstream reference implementation. Future code suggestions should consider maintaining this alignment to ensure consistency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build (1.24.x, windows-latest)
- GitHub Check: Build (1.24.x, macos-13)
- GitHub Check: Build (1.24.x, ubuntu-latest)
- GitHub Check: Build (1.23.x, macos-latest)
- GitHub Check: Build (1.24.x, macos-latest)
- GitHub Check: Analyse
- GitHub Check: Build (1.23.x, ubuntu-latest)
- GitHub Check: Build (1.23.x, macos-13)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (19)
parse_test.go (10)
29-39: Excellent migration to error-based API.The test has been correctly updated to use the new error-based parsing API. The pattern of checking
err == nilinstead of a boolean success flag is idiomatic Go and provides better error information.
45-47: Proper error assertion for whitespace input.Good use of
require.Error(t, err)to assert that parsing fails for input with leading whitespace, which is the expected behavior.
56-69: Consistent error handling in benchmarks.The benchmark properly handles errors from the new API by calling
b.Fatalwhen parsing fails, ensuring benchmark validity while maintaining performance measurement accuracy.
103-113: Test coverage maintains comprehensive validation.The ParseInt test correctly validates both positive and negative edge cases with proper error checking. The transition from boolean to error-based validation maintains the same test coverage quality.
121-127: Proper handling of sign-only input edge cases.Good validation that inputs containing only "+" or "-" characters properly return errors, which is the expected behavior for invalid numeric input.
133-135: Whitespace validation correctly implemented.The test properly verifies that leading whitespace causes parsing to fail with an error, maintaining strict input validation.
156-169: Benchmark error handling follows best practices.All benchmark functions consistently handle errors from the updated parsing API, ensuring accurate performance measurements while preventing silent failures.
197-207: Integer type-specific parsing maintains precision.The ParseInt32 test correctly validates range limits and error conditions for 32-bit integers, ensuring type safety is preserved in the new error-based API.
581-599: Comprehensive float parsing validation with enhanced error context.The test provides excellent coverage of float64 parsing edge cases including scientific notation, special values, and malformed inputs. The addition of input context in error messages (
"input: %s", tt.in) improves debugging capabilities.
678-702: Float32 precision and special value handling.Excellent test coverage for float32-specific edge cases including negative zero detection and precision limits. The
negzerofield and corresponding validation logic properly handles IEEE 754 floating-point special cases.parse.go (9)
5-5: Good addition of strconv import for error handling.The strconv package import is necessary to support the new error-based API using
strconv.NumError,strconv.ErrSyntax, andstrconv.ErrRange.
20-22: Excellent migration to error-based API.The function signature change from
(uint64, bool)to(uint64, error)follows Go idioms and provides better error information. The delegation toparseUnsignedwith proper function name is well-implemented.
26-28: Good error-based implementation for signed integers.The ParseInt function properly delegates to
parseSignedwith correct parameters and function name for error reporting.
62-76: Excellent error handling in digit parsing.The
parseDigitsfunction properly returnsstrconv.ErrSyntaxfor invalid characters andstrconv.ErrRangefor overflow conditions. The overflow detection usingnn < nis a clever way to detect uint64 overflow.
80-119: Comprehensive signed integer parsing with proper error reporting.The
parseSignedfunction handles all edge cases correctly:
- Empty input validation
- Sign character handling
- Overflow/underflow detection
- Proper error construction with
strconv.NumErrorThe logic for handling negative numbers and range checking is well-implemented.
123-140: Well-implemented unsigned integer parsing.The
parseUnsignedfunction correctly handles unsigned integer constraints by rejecting sign prefixes and properly checking for overflow against the maximum range.
145-250: Comprehensive float parsing with robust error handling.The
parseFloatfunction provides excellent coverage of floating-point parsing:
- Sign handling
- Integer and fractional parts
- Scientific notation with exponent
- Overflow detection and infinite/NaN checking
- Proper error reporting for all failure cases
The implementation is thorough and follows IEEE 754 standards.
254-256: Clean delegation for float64 parsing.Good implementation that delegates to the generic
parseFloatfunction with the correct function name.
260-269: Proper float32 range validation.The ParseFloat32 function correctly validates that the parsed float64 value fits within float32 range before conversion, ensuring no silent overflow occurs.
There was a problem hiding this comment.
Code Review
This pull request refactors the parsing functions to return an error instead of a boolean, which is a great improvement that aligns with Go's idiomatic error handling. The implementation is solid, using strconv.NumError to provide detailed error information.
I've identified a few minor issues where an incorrect function name is used when creating errors, which could lead to slightly misleading error messages. I've left suggestions to correct these. The test files have been updated correctly to reflect the new function signatures.
Overall, this is a good change that improves code quality and maintainability.
error instead of boolean return
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
parse.go (6)
20-21: LGTM! Function signature correctly updated.The function signature properly changed to return
(uint64, error)and the function name passed toparseUnsignedis correct.
26-27: LGTM! Function signature correctly updated.The function signature properly changed to return
(int64, error)and the function name passed toparseSignedis correct.
31-32: LGTM! Function names in error reporting are now correct.All signed integer parsing functions now pass their specific function names ("ParseInt32", "ParseInt16", "ParseInt8") instead of the generic "ParseInt". This ensures accurate error reporting.
Also applies to: 36-37, 41-42
46-47: LGTM! Function names in error reporting are now correct.All unsigned integer parsing functions now pass their specific function names ("ParseUint32", "ParseUint16", "ParseUint8") instead of the generic "ParseUint". This ensures accurate error reporting.
Also applies to: 51-52, 56-57
254-255: LGTM! Function name in error reporting is correct.The
ParseFloat64function now passes the correct function name for accurate error reporting.
260-268: LGTM! Function properly handles float32 range checking.The
ParseFloat32function correctly:
- Uses the proper function name in error reporting
- Handles overflow conditions for float32 range
- Returns appropriate
strconv.NumErrorfor range violations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parse.go(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
PR: gofiber/fiber#2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-07-01T15:49:51.308Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
parse.go (4)
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-02T22:57:30.345Z
Learning: In the `GetByID` method, when handling a decode error, use `decodeErr` in the error message to accurately report the decoding failure.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `GetByID` method, when handling a decode error, use `decodeErr` in the error message to accurately report the decoding failure.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:15-20
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Ensure to check the function signature and parameters before suggesting changes to avoid redundant recommendations.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:15-20
Timestamp: 2024-07-01T15:48:33.417Z
Learning: Ensure to check the function signature and parameters before suggesting changes to avoid redundant recommendations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Analyse
- GitHub Check: Build (1.23.x, macos-latest)
- GitHub Check: Compare
- GitHub Check: Build (1.24.x, macos-13)
- GitHub Check: Build (1.24.x, ubuntu-latest)
- GitHub Check: Build (1.23.x, macos-13)
- GitHub Check: Build (1.24.x, macos-latest)
- GitHub Check: Build (1.24.x, windows-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.23.x, ubuntu-latest)
🔇 Additional comments (5)
parse.go (5)
5-5: LGTM! Proper import added for error handling.The addition of the
strconvimport is necessary to support the new error-based parsing API usingstrconv.NumErrorand error constants.
60-76: LGTM! Proper error handling in digit parsing.The
parseDigitsfunction correctly returnsstrconv.ErrSyntaxfor non-digit characters andstrconv.ErrRangefor overflow conditions, following Go's standard error conventions.
78-119: LGTM! Comprehensive error handling in signed integer parsing.The
parseSignedfunction properly handles all error conditions usingstrconv.NumErrorwith correct function names, input strings, and error types. The overflow/underflow logic is sound.
121-140: LGTM! Proper error handling in unsigned integer parsing.The
parseUnsignedfunction correctly handles error conditions and overflow checks usingstrconv.NumErrorwith accurate error reporting.
142-250: LGTM! Comprehensive floating-point parsing with proper error handling.The
parseFloatfunction handles all error conditions appropriately:
- Empty input and syntax errors
- Overflow in integer part
- Too many fractional digits
- Invalid exponent syntax
- Infinite/NaN results
All errors are properly wrapped in
strconv.NumErrorwith correct function names.
error instead of boolean return
error instead of boolean return
error instead of boolean return
Summary by CodeRabbit
Refactor
Tests
Documentation