feat: Add nlreturn linter and clean up code formatting #111
Conversation
WalkthroughThis pull request introduces several code improvements across multiple packages. The changes include updating the CI configuration to enable a new linter ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AddQueryParams
participant Validator
Caller->>AddQueryParams: Call AddQueryParams(url, params)
AddQueryParams->>Validator: For each key-value, call validateKeyValue(key, value)
alt Valid key-value pair
Validator-->>AddQueryParams: Returns nil (no error)
AddQueryParams-->>Caller: Returns updated URL
else Invalid key-value pair
Validator-->>AddQueryParams: Returns error
AddQueryParams-->>Caller: Returns error
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 is an option to exclude specific linters for specific files. I would suggest disabling/excluding |
Closes kashifkhan0771#104 exclude test files
giuszeppe
force-pushed
the
feat/enable-nlreturn-linter
branch
from
253e4ca to
922857c
Compare
|
I added an issues section with the exclude rule, to exclude our nlreturn from the test files. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
time/time.go (4)
36-51: Consider improving time difference readability.The function logic is correct, but the code structure could be simplified to improve readability.
Consider this refactoring:
func TimeDifferenceHumanReadable(from, to time.Time) string { diff := to.Sub(from) + isNegative := diff < 0 if diff < 0 { diff = -diff - if diff.Hours() > 24 { - return fmt.Sprintf("%d day(s) ago", int(diff.Hours()/24)) - } - - return fmt.Sprintf("in %d hour(s)", int(diff.Hours())) } - if diff.Hours() > 24 { - return fmt.Sprintf("in %d day(s)", int(diff.Hours()/24)) - } - - return fmt.Sprintf("in %d hour(s)", int(diff.Hours())) + + format := "in %d %s" + if isNegative { + format = "%d %s ago" + } + + if diff.Hours() > 24 { + return fmt.Sprintf(format, int(diff.Hours()/24), "day(s)") + } + return fmt.Sprintf(format, int(diff.Hours()), "hour(s)") }
138-145: Consider using named return values consistently.The function uses named return parameters but doesn't utilize them in the return statement.
Consider either:
- Using the named return values throughout the function:
func SplitDuration(d time.Duration) (days, hours, minutes, seconds int) { days = int(d.Hours()) / 24 hours = int(d.Hours()) % 24 minutes = int(d.Minutes()) % 60 seconds = int(d.Seconds()) % 60 - return + return days, hours, minutes, seconds }Or 2. Removing the named return values if they don't provide additional clarity:
-func SplitDuration(d time.Duration) (days, hours, minutes, seconds int) { +func SplitDuration(d time.Duration) (int, int, int, int) { days := int(d.Hours()) / 24 hours := int(d.Hours()) % 24 minutes := int(d.Minutes()) % 60 seconds := int(d.Seconds()) % 60 return days, hours, minutes, seconds }
189-204: Consider enhancing query parameter validation.The validation is good but could be more comprehensive.
Consider:
- Adding length limits for keys and values
- Supporting additional safe characters (e.g., dots, underscores)
- Using a more descriptive regex constant name
-const alphaNumericRegex = "^[a-zA-Z0-9-]+$" +const queryParamRegex = "^[a-zA-Z0-9._-]{1,64}$" func validateKeyValue(key, value string) error { if key == "" { return errors.New("query parameter key cannot be empty") } if value == "" { return fmt.Errorf("query parameter value for key %s cannot be empty", key) } - if !alphaNumericRe.MatchString(key) { - return fmt.Errorf("query parameter key %s must be alphanumeric", key) + if !queryParamRe.MatchString(key) { + return fmt.Errorf("query parameter key %s must be alphanumeric and not exceed 64 characters", key) } - if !alphaNumericRe.MatchString(value) { - return fmt.Errorf("query parameter value %s for key %s must be alphanumeric", value, key) + if !queryParamRe.MatchString(value) { + return fmt.Errorf("query parameter value %s for key %s must be alphanumeric and not exceed 64 characters", value, key) } return nil }
240-258: Consider improving scheme validation efficiency.The current implementation iterates through all schemes even after finding a match.
Consider using a map for O(1) lookup:
func IsValidURL(urlStr string, allowedReqSchemes []string) bool { if urlStr == "" { return false } parsedURL, err := url.Parse(urlStr) if err != nil { return false } + allowedSchemes := make(map[string]struct{}, len(allowedReqSchemes)) for _, scheme := range allowedReqSchemes { if scheme == "" { return false } - if parsedURL.Scheme == scheme { - return true - } + allowedSchemes[scheme] = struct{}{} } - return false + _, ok := allowedSchemes[parsedURL.Scheme] + return ok }url/url.go (2)
79-122: Consider using early returns for cleaner error handling.The function's error handling could be simplified by using early returns instead of collecting error messages in a slice. This would align with the nlreturn linter's recommendations while maintaining the same validation logic.
Here's a suggested refactoring:
func BuildURL(scheme, host, path string, query map[string]string) (string, error) { - var errMessage []string if scheme == "" { - errMessage = append(errMessage, "scheme is required") + return "", errors.New("scheme is required") } + if host == "" { - errMessage = append(errMessage, "host is required") + return "", errors.New("host is required") } + if host != "" { if !hostRe.MatchString(host) { - errMessage = append(errMessage, "the host is not valid") + return "", errors.New("the host is not valid") } } if path != "" { if !pathRe.MatchString(path) { - errMessage = append(errMessage, "path is permitted with a-z, 0-9, - and _ characters and multiple path segments") + return "", errors.New("path is permitted with a-z, 0-9, - and _ characters and multiple path segments") } } - if errMessage != nil { - return "", errors.New(strings.Join(errMessage, "; ")) - } // Rest of the function remains the same
347-361: Consider clarifying behavior for multiple parameter values.While the function correctly returns the first value when multiple values exist for a parameter, this behavior should be documented explicitly in the function's documentation.
Add a note to the documentation:
// Notes: // - This function uses the `net/url` package for robust URL parsing. // - It assumes the URL is properly formatted with query parameters starting after a "?". +// - If multiple values exist for the same parameter, only the first value is returned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.golangci.yml(1 hunks)boolean/boolean.go(8 hunks)caching/caching.go(3 hunks)caching/caching_test.go(2 hunks)ctxutils/ctxutils.go(2 hunks)errutils/errutils.go(1 hunks)fsutils/fsutils_test.go(1 hunks)logging/logging.go(1 hunks)math/math.go(7 hunks)pointers/common.go(3 hunks)pointers/numeric.go(14 hunks)pointers/text.go(2 hunks)pointers/text_test.go(1 hunks)rand/rand.go(2 hunks)rand/rand_test.go(9 hunks)slice/slice_test.go(2 hunks)strings/strings.go(5 hunks)structs/structs_test.go(1 hunks)templates/html_test.go(1 hunks)templates/text_test.go(1 hunks)time/time.go(5 hunks)time/time_test.go(4 hunks)url/url.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- errutils/errutils.go
- pointers/common.go
- templates/html_test.go
- templates/text_test.go
- structs/structs_test.go
- rand/rand.go
- pointers/text.go
- caching/caching_test.go
- ctxutils/ctxutils.go
- pointers/text_test.go
- fsutils/fsutils_test.go
- slice/slice_test.go
- rand/rand_test.go
- pointers/numeric.go
- caching/caching.go
- logging/logging.go
- math/math.go
- time/time_test.go
- strings/strings.go
- boolean/boolean.go
🔇 Additional comments (7)
.golangci.yml (2)
12-12: Addition ofnlreturnlinter.
You've correctly enabled the newnlreturnlinter in thelinters.enablelist to improve code quality checks.
13-19: Exclusion rule for test files.
The newissues.exclude-rulessection correctly excludes files matching the_test\.gopattern from being checked bynlreturn. This aligns with the PR objective to preserve existing formatting in test files while enforcing stricter checks elsewhere.url/url.go (5)
16-31: LGTM! Well-structured regex definitions with clear documentation.The regex patterns are well-defined and properly compiled using
MustCompile. The documentation clearly explains the validation rules for each pattern.
165-187: LGTM! Robust URL validation with proper error handling.The function implements thorough validation including:
- URL parsing validation
- Scheme validation against an allowed list
- Query parameter validation using the new
validateKeyValuefunction
189-204: LGTM! Well-implemented validation with clear error messages.The function provides thorough validation of query parameters with:
- Empty string checks for both keys and values
- Alphanumeric validation using regex
- Descriptive error messages that include the problematic values
240-258: LGTM! Thorough URL validation with proper scheme checking.The function implements proper validation with:
- Early returns for invalid cases
- Explicit empty string checks
- Comprehensive scheme validation
294-310: LGTM! Secure domain extraction with proper error handling.The function uses the
publicsuffixpackage for secure domain extraction and implements thorough error handling.
Closes #104
Description:
I enabled the nlreturn linter and cleaned up all the code that needed formatting.
Some parts of the code were marked as wrong by the linter, anyhow, for how it is structured, I don't think it should be changed.
An example of this code is the following:
Tell me if I should change the format of all this anonymous functions as well, they're mainly in the test file, just to let you understand what I'm talking about
Checklist:
make test.make lint.Summary by CodeRabbit
New Features
Bug Fixes
Refactor & Style