BE-636-domain-packages | Add number package, introduce http helper #572
Conversation
To address cyclic import issues we move several utility functions to more focused packages.
deividaspetraitis
requested review from
cryptomatictrader,
PaddyMc and
p0mvn
|
WalkthroughThis pull request introduces a new function, Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (9)
delivery/http/helper.go (2)
9-13: Consider enhancing the documentation with valid values and examples.While the documentation is good, it could be more helpful by specifying the valid boolean values and including usage examples.
// ParseBooleanQueryParam parses a boolean query parameter. // Returns false if the parameter is not present. -// Errors if the value is not a valid boolean. +// Returns an error if the value is not a valid boolean ("true", "false", "1", "0", "t", "f"). // Returns the boolean value and an error if any. +// +// Example: +// // For request: GET /api?enabled=true +// val, err := ParseBooleanQueryParam(c, "enabled") +// // val == true, err == nil
13-23: Consider adding input validation and whitespace handling.The core implementation is solid, but could be enhanced with additional safeguards.
func ParseBooleanQueryParam(c echo.Context, paramName string) (paramValue bool, err error) { + if paramName == "" { + return false, fmt.Errorf("parameter name cannot be empty") + } paramValueStr := c.QueryParam(paramName) if paramValueStr != "" { + paramValueStr = strings.TrimSpace(paramValueStr) paramValue, err = strconv.ParseBool(paramValueStr) if err != nil { return false, err } } return paramValue, nil }Don't forget to add the required imports:
import ( "fmt" "strings" )domain/number/number.go (5)
1-2: Enhance package documentationConsider expanding the package documentation to include:
- Common use cases
- Examples of usage
- Any important considerations or limitations
Example enhancement:
-// Package number provides utility functions for working with numbers. +// Package number provides utility functions for parsing and converting string-formatted numbers +// into various numeric types. It supports parsing of comma-separated lists of numbers and +// provides both specific (uint64) and generic number parsing capabilities. +// +// Example usage: +// numbers, err := number.ParseNumbers("1,2,3") +// // numbers = []uint64{1, 2, 3}
9-23: Improve function documentation and performanceThe function implementation is correct, but could benefit from:
- More detailed documentation including examples and error cases
- Pre-allocation of the slice capacity for better performance
Consider these improvements:
-// ParseNumbers parses a comma-separated list of numbers into a slice of unit64. +// ParseNumbers parses a comma-separated list of numbers into a slice of uint64. +// It accepts a string of comma-separated numbers (e.g., "1,2,3" or "1, 2, 3") +// and returns a slice of uint64 values. Empty strings in the input are ignored. +// +// Returns an error if any number in the list: +// - Cannot be parsed as uint64 +// - Is negative +// - Exceeds uint64 maximum value +// +// Example: +// numbers, err := ParseNumbers("1,2,3") +// if err != nil { +// // handle error +// } +// // numbers = []uint64{1, 2, 3} func ParseNumbers(numbersParam string) ([]uint64, error) { var numbers []uint64 numStrings := splitAndTrim(numbersParam, ",") + numbers = make([]uint64, 0, len(numStrings))
25-39: Enhance generic function documentation and performanceThe generic implementation is well-designed, but could benefit from:
- More detailed documentation with concrete examples
- Pre-allocation of the slice capacity
Consider these improvements:
-// ParseNumberType parses a comma-separated list of numbers into a slice of the specified type. +// ParseNumberType is a generic function that parses a comma-separated list of numbers +// into a slice of the specified type T. It accepts a parsing function that defines +// how to convert a string to type T. +// +// Parameters: +// - numbersParam: A comma-separated string of numbers (e.g., "1,2,3" or "1, 2, 3") +// - parseFn: A function that converts a string to type T and returns an error if parsing fails +// +// Returns a slice of type T and an error if any parsing fails. +// +// Example: +// // Parse to int64 +// parseFn := func(s string) (int64, error) { return strconv.ParseInt(s, 10, 64) } +// numbers, err := ParseNumberType("1,2,3", parseFn) +// +// // Parse to float64 +// numbers, err := ParseNumberType("1.1,2.2", strconv.ParseFloat) func ParseNumberType[T any](numbersParam string, parseFn func(s string) (T, error)) ([]T, error) { numStrings := splitAndTrim(numbersParam, ",") - var numbers []T + numbers := make([]T, 0, len(numStrings))
41-51: Clarify helper function documentationThe implementation is correct, but the documentation should be more specific about empty string handling.
Consider this documentation improvement:
-// splitAndTrim splits a string by a separator and trims the resulting strings. +// splitAndTrim splits a string by a separator, trims whitespace from the resulting +// substrings, and filters out empty strings from the result. +// +// Example: +// result := splitAndTrim("a, b,, c ", ",") +// // result = []string{"a", "b", "c"}
1-51: Overall implementation successfully achieves PR objectivesThe new
numberpackage effectively resolves the cyclic import issue by providing a dedicated location for number parsing utilities. The implementation is clean, well-structured, and provides both specific (uint64) and generic number parsing capabilities.The code successfully:
- Moves number parsing logic to a dedicated package
- Maintains the original functionality
- Provides flexibility through generic implementation
Consider adding unit tests to verify edge cases such as:
- Empty input strings
- Malformed numbers
- Boundary values (max uint64)
- Various separators in generic function
Would you like me to help with generating these tests?router/types/get_direct_custom_quote_request.go (2)
8-9: LGTM! Import changes align with architecture goals.The addition of the HTTP package import while maintaining the domain package import properly supports the architectural goal of separating concerns and resolving cyclic dependencies.
29-32: Consider wrapping the boolean parsing error.To improve error handling, consider wrapping the ParseBooleanQueryParam error with a domain-specific error type. This would make it easier for upstream code to handle specific error cases.
r.ApplyExponents, err = http.ParseBooleanQueryParam(c, "applyExponents") if err != nil { - return err + return fmt.Errorf("invalid applyExponents parameter: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
delivery/http/helper_test.gois excluded by!**/*_test.godomain/number/number_test.gois excluded by!**/*_test.godomain/router_handlet_test.gois excluded by!**/*_test.go
📒 Files selected for processing (6)
delivery/http/helper.go(1 hunks)domain/number/number.go(1 hunks)domain/util.go(0 hunks)pools/delivery/http/pools_handler.go(3 hunks)router/types/get_direct_custom_quote_request.go(2 hunks)router/types/get_quote_request.go(3 hunks)
💤 Files with no reviewable changes (1)
- domain/util.go
🔇 Additional comments (6)
delivery/http/helper.go (2)
1-8: LGTM!
The package name and imports are appropriate and minimal.
13-23: Verify the function usage across the codebase.
Let's confirm that all callers are using this new helper function correctly.
✅ Verification successful
Function usage is consistent across the codebase
The verification confirms that ParseBooleanQueryParam is being used correctly in all expected locations:
router/types/get_quote_request.go: Used forsingleRoute,applyExponents, andappendBaseFeeparametersrouter/types/get_direct_custom_quote_request.go: Used forapplyExponentsparameterpools/delivery/http/pools_handler.go: Used forwith_market_incentivesparameter
The search for any remaining direct boolean parsing (QueryParam.*ParseBool) found no other instances, indicating complete adoption of this helper function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ParseBooleanQueryParam across the codebase
# Expected: Find usage in pools_handler.go and quote request files
# Search for direct function calls
echo "Searching for ParseBooleanQueryParam usage:"
rg -A 2 'ParseBooleanQueryParam\('
# Search for any remaining boolean parsing that should be using this helper
echo -e "\nSearching for potential missed opportunities to use ParseBooleanQueryParam:"
rg -A 2 'QueryParam.*ParseBool'
Length of output: 2077
router/types/get_direct_custom_quote_request.go (1)
29-29: LGTM! Verify consistent usage across codebase.
The switch to http.ParseBooleanQueryParam aligns with the architectural changes. Let's verify this pattern is consistently applied across the codebase.
✅ Verification successful
Consistent usage of http.ParseBooleanQueryParam confirmed
All instances of boolean query parameter parsing now use the http package implementation, with no remaining references to domain.ParseBooleanQueryParam. The pattern is consistently applied across the codebase in the following locations:
pools/delivery/http/pools_handler.go:withMarketIncentivesparameterrouter/types/get_direct_custom_quote_request.go:applyExponentsparameterrouter/types/get_quote_request.go: Multiple parameters (singleRoute,applyExponents,appendBaseFee)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that domain.ParseBooleanQueryParam is no longer used anywhere
# and http.ParseBooleanQueryParam is used consistently
echo "Checking for any remaining usage of domain.ParseBooleanQueryParam:"
rg "domain\.ParseBooleanQueryParam"
echo -e "\nVerifying consistent usage of http.ParseBooleanQueryParam:"
rg "http\.ParseBooleanQueryParam" -A 2
Length of output: 1523
router/types/get_quote_request.go (1)
7-7: LGTM! Import change aligns with PR objectives.
The addition of the http package import supports the refactoring goal of moving parsing functions to the http layer, helping resolve the cyclic dependency issues.
pools/delivery/http/pools_handler.go (2)
11-14: LGTM! Import changes align with architectural goals.
The new imports properly reflect the reorganization of utility functions into specialized packages, helping resolve cyclic dependencies.
80-83: LGTM! Utility function migration implemented correctly.
The changes properly utilize the new specialized packages for parsing query parameters while maintaining consistent error handling.
Let's verify the consistent usage of these new utility functions across the codebase:
Also applies to: 90-93
✅ Verification successful
This will help us ensure that all query parameter parsing is consistently using the new utility functions and identify if there are other handlers that might need similar changes.
Migration to new utility functions is correctly implemented, but similar changes could be beneficial in other handlers
The verification shows that:
- No legacy
domain.ParseBooleanQueryParamordomain.ParseNumbersusages remain - The new utility functions are properly used in pools_handler.go
- Several other handlers in the codebase use direct query parameter parsing that could benefit from similar utility function usage, particularly in:
- tokens/delivery/http/tokens_delivery.go
- router/delivery/http/router_handler.go
- passthrough/delivery/http/passthrough_handler.go
While this change is correct and complete for the pools handler, consider creating a follow-up ticket to standardize query parameter parsing across all handlers using these new utility functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new utility functions
# Check for any remaining usage of old parsing functions
echo "Checking for any remaining usage of old parsing functions..."
rg "domain\.ParseBooleanQueryParam|domain\.ParseNumbers" -l
# Verify consistent usage of new functions
echo "Verifying usage of new utility functions..."
rg "deliveryhttp\.ParseBooleanQueryParam|number\.ParseNumbers" -A 2
Length of output: 1106
Script:
#!/bin/bash
# Let's verify if there are any other query parameter parsing functions in the codebase
# that might need similar migration
# Check for any query parameter parsing patterns
echo "Checking for query parameter parsing patterns..."
rg "ParseBooleanQueryParam|ParseNumbers|c\.QueryParam|c\.Param" -A 2
# Check for any files in the delivery/http package that might need similar changes
echo "Checking files in delivery/http package..."
fd -t f . "delivery/http"
Length of output: 11407
This PR represents a smaller, more focused part of bigger PR #553 implementing sorting, filtering, pagination and search functionality for /pools endpoint. For bigger implementation picture refer to mentioned PR and for more context please refer to BE-636.
To address cyclic import issues we move several utility functions to more focused packages. This issue arrised when pools/http.go imported
domainpackage and at the same timedomainpackage imported api.GetPoolsRequestFilter. To resolve this issue we did following:domain.ParseBooleanQueryParamis moved tohttp.ParseBooleanQueryParam. As side effectdomainpackage is now not aware ofhttplayer and it's dependencies.domain.ParseNumbersto is moved tonumber.ParseNumbersSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
delivery/httppackage.These updates aim to streamline data handling and improve the overall functionality of the application.