fix: use idiomatic Go boolean checks in store_valkey.go#317
fix: use idiomatic Go boolean checks in store_valkey.go#317HarshitPal25 wants to merge 1 commit into
Conversation
Replace `disableCache == true` with `disableCache` and `forceSingleCache == true` with `forceSingleCache` per Go style conventions. The Go Code Review Comments wiki recommends against comparing boolean values to true/false explicitly. Signed-off-by: HarshitPal25 <harshit13082006@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors boolean comparisons in pkg/store/store_valkey.go to use idiomatic Go syntax. The reviewer suggests enhancing the error handling for the VALKEY_DISABLE_CACHE and VALKEY_FORCE_SINGLE environment variables, as parsing errors are currently silently ignored, which could lead to unexpected configuration behavior.
| if valkeyDisableCache != "" { | ||
| disableCache, err := strconv.ParseBool(valkeyDisableCache) | ||
| if err == nil && disableCache == true { | ||
| if err == nil && disableCache { |
There was a problem hiding this comment.
While refactoring for idiomatic boolean checks, it's a good opportunity to improve error handling. Currently, if VALKEY_DISABLE_CACHE contains an invalid boolean string, strconv.ParseBool returns an error which is silently ignored. Given that other configuration errors in this function (like missing VALKEY_ADDR) cause a failure, it would be more consistent and robust to return an error here as well.
if err != nil {
return nil, fmt.Errorf("invalid VALKEY_DISABLE_CACHE %q: %w", valkeyDisableCache, err)
}
if disableCache {| if valkeyForceSingle != "" { | ||
| forceSingleCache, err := strconv.ParseBool(valkeyForceSingle) | ||
| if err == nil && forceSingleCache == true { | ||
| if err == nil && forceSingleCache { |
There was a problem hiding this comment.
Similar to the VALKEY_DISABLE_CACHE check, handling the error from strconv.ParseBool for VALKEY_FORCE_SINGLE prevents silent configuration failures and maintains consistency with how other environment variables are processed in this function.
if err != nil {
return nil, fmt.Errorf("invalid VALKEY_FORCE_SINGLE %q: %w", valkeyForceSingle, err)
}
if forceSingleCache {There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup in the Valkey store configuration path (pkg/store/store_valkey.go) by replacing explicit boolean comparisons (== true) with idiomatic Go boolean checks when applying options from environment variables.
Changes:
- Simplified
ParseBoolresult checks forVALKEY_DISABLE_CACHEandVALKEY_FORCE_SINGLEto use idiomaticif err == nil && flagstyle. - Kept behavior the same while aligning with common Go lint recommendations.
| if valkeyDisableCache != "" { | ||
| disableCache, err := strconv.ParseBool(valkeyDisableCache) | ||
| if err == nil && disableCache == true { | ||
| if err == nil && disableCache { | ||
| valkeyClientOptions.DisableCache = true | ||
| klog.Info("valkeyClientOptions DisableCache is set to true") | ||
| } |
| if valkeyForceSingle != "" { | ||
| forceSingleCache, err := strconv.ParseBool(valkeyForceSingle) | ||
| if err == nil && forceSingleCache == true { | ||
| if err == nil && forceSingleCache { | ||
| valkeyClientOptions.ForceSingleClient = true |
Description:
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Replaces explicit boolean comparisons with idiomatic Go style in
store_valkey.go:if err == nil && disableCache == true→if err == nil && disableCacheif err == nil && forceSingleCache == true→if err == nil && forceSingleCacheThe Go Code Review Comments wiki and
golint/revivelinters recommend against comparing boolean values totrue/falseexplicitly.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: