refactor: replace usages of golang.org/x/exp with stdlib
#1828
Conversation
- golang.org/x/exp/rand: Available in Go 1.22 [2] - golang.org/x/exp/slices: Available in Go 1.21 [3] - golang.org/x/exp/slog: Available in Go 1.21 [1] [1]: https://go.dev/doc/go1.21#slog [2]: https://go.dev/doc/go1.22#math_rand_v2 [3]: https://go.dev/doc/go1.21#slices Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on updating import statements to utilize the standard library's Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
go.mod(1 hunks)internal/engines/subjectFilter.go(1 hunks)internal/engines/subjectPermission.go(1 hunks)internal/storage/context/attributes.go(1 hunks)internal/storage/context/tuples.go(1 hunks)internal/storage/context/tuples_test.go(1 hunks)internal/storage/memory/dataReader.go(1 hunks)internal/storage/memory/utils/filter.go(1 hunks)internal/storage/postgres/utils/common.go(1 hunks)pkg/database/postgres/postgres.go(1 hunks)pkg/development/coverage/coverage.go(1 hunks)pkg/tuple/tuple.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- pkg/development/coverage/coverage.go
- go.mod
- internal/storage/context/attributes.go
🔇 Additional comments (10)
internal/storage/postgres/utils/common.go (2)
9-9: LGTM: Appropriate stdlib package replacement
The change from golang.org/x/exp/rand to math/rand/v2 aligns with the PR objective and uses the stable stdlib package available since Go 1.22.
Line range hint 156-165: LGTM: Correct implementation of exponential backoff with jitter
The function correctly implements exponential backoff with jitter using the new stdlib rand package. The implementation includes:
- Proper exponential growth with capping
- Appropriate jitter range (0-50%)
- Context cancellation handling
- Informative logging
internal/engines/subjectFilter.go (1)
7-7: LGTM!
The import statement has been updated to use the standard library slices package, aligning with the PR objectives and Go 1.21+ features.
internal/storage/memory/utils/filter.go (1)
4-5: LGTM!
Updated the import to use the standard library slices package. The usage of slices.Contains is appropriate.
internal/storage/context/tuples.go (1)
4-4: LGTM!
The import statement for slices has been updated to the standard library version. The slices.Contains function is used correctly.
internal/engines/subjectPermission.go (1)
6-6: LGTM!
Replaced the golang.org/x/exp/slices import with the standard library slices package. Usage within the code is consistent and correct.
pkg/tuple/tuple.go (1)
6-6: LGTM: Import change aligns with Go 1.23 upgrade
The change from golang.org/x/exp/slices to stdlib slices is correct and maintains the same functionality.
internal/storage/context/tuples_test.go (1)
4-4: LGTM: Test coverage validates the slices package migration
The change to stdlib slices is well-tested across multiple scenarios, ensuring the migration's correctness.
internal/storage/memory/dataReader.go (2)
6-6: LGTM: Import change maintains pagination functionality
The migration to stdlib slices is correct and preserves the existing pagination and filtering behavior in QueryUniqueSubjectReferences.
6-6: Verify complete removal of experimental slices package
Let's ensure all usages of the experimental package have been replaced.
✅ Verification successful
Since the previous search didn't find any remaining instances of golang.org/x/exp/slices, let's verify the stdlib slices usage with a different pattern to ensure we catch all cases.
Migration to stdlib slices package is complete
The verification shows that:
- No instances of
golang.org/x/exp/slicesimports remain in the codebase - All
slices.Containscalls are using the standard library version, as evidenced by the simple"slices"imports and consistent usage patterns across multiple files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining golang.org/x/exp/slices imports
# and verify that all slices.Contains calls use the stdlib version
# Check for any remaining experimental imports
echo "Checking for remaining experimental imports..."
rg -l "golang.org/x/exp/slices"
# Check for all slices.Contains usage to ensure they're using stdlib
echo "Verifying slices.Contains usage..."
ast-grep --pattern 'import "slices"
$$$
slices.Contains($$$)'
Length of output: 277
Script:
#!/bin/bash
# Search for any slices package usage to verify correct implementation
echo "Checking all slices package imports and usage..."
rg -A 2 '^import.*"slices"'
# Also check for any potential direct usage without import
echo "Checking for any slices.Contains calls..."
rg "slices\.Contains"
Length of output: 2788
| import ( | ||
| "context" | ||
| "fmt" | ||
| "log/slog" |
There was a problem hiding this comment.
Correct the usage of logging methods in log/slog.
The standard library log/slog package does not have slog.Warn and slog.Info methods directly. Instead, logging at different levels should use a slog.Logger instance or slog.Log with the appropriate slog.Level.
Apply this diff to fix the logging statements:
func setDefaultQueryExecMode(config *pgx.ConnConfig) {
// Default mode if no specific mode is found in the connection string
defaultMode := "cache_statement"
// Iterate through the map keys to check if any are mentioned in the connection string
for key := range queryExecModes {
if strings.Contains(config.ConnString(), "default_query_exec_mode="+key) {
config.DefaultQueryExecMode = queryExecModes[key]
- slog.Info("setDefaultQueryExecMode", slog.String("mode", key))
+ slog.Log(context.Background(), slog.LevelInfo, "setDefaultQueryExecMode", slog.String("mode", key))
return
}
}
// Set to default mode if no matching mode is found
config.DefaultQueryExecMode = queryExecModes[defaultMode]
- slog.Warn("setDefaultQueryExecMode", slog.String("mode", defaultMode))
+ slog.Log(context.Background(), slog.LevelWarn, "setDefaultQueryExecMode", slog.String("mode", defaultMode))
}
func setPlanCacheMode(config *pgx.ConnConfig) {
// Default plan cache mode
const defaultMode = "auto"
// Extract connection string
connStr := config.ConnString()
planCacheMode := defaultMode
// Check for specific plan cache modes in the connection string
for key, value := range planCacheModes {
if strings.Contains(connStr, "plan_cache_mode="+key) {
if key == "disable" {
delete(config.Config.RuntimeParams, "plan_cache_mode")
- slog.Info("setPlanCacheMode", slog.String("mode", "disabled"))
+ slog.Log(context.Background(), slog.LevelInfo, "setPlanCacheMode", slog.String("mode", "disabled"))
return
}
planCacheMode = value
- slog.Info("setPlanCacheMode", slog.String("mode", key))
+ slog.Log(context.Background(), slog.LevelInfo, "setPlanCacheMode", slog.String("mode", key))
break
}
}
// Set the plan cache mode
config.Config.RuntimeParams["plan_cache_mode"] = planCacheMode
if planCacheMode == defaultMode {
- slog.Warn("setPlanCacheMode", slog.String("mode", defaultMode))
+ slog.Log(context.Background(), slog.LevelWarn, "setPlanCacheMode", slog.String("mode", defaultMode))
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
log/slog package does have slog.Warn and slog.Info.
There was a problem hiding this comment.
@Juneezee, thank you for the correction. You are absolutely right; the log/slog package does have slog.Warn and slog.Info methods. I apologize for the confusion.
✏️ Learnings added
Learnt from: Juneezee
PR: Permify/permify#1828
File: pkg/database/postgres/postgres.go:6-6
Timestamp: 2024-12-06T17:47:06.068Z
Learning: The standard library `log/slog` package provides the `slog.Warn` and `slog.Info` methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Hi @Juneezee, thank you for your contribution 🚀 |
These experimental packages are available in the Go standard library already. We have upgraded our Go version to 1.23 in #1483 so we can replace them with the standard library.
golang.org/x/exp/rand: Available in Go 1.22 1golang.org/x/exp/slices: Available in Go 1.21 2golang.org/x/exp/slog: Available in Go 1.21 3Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
slicespackage across multiple files, enhancing maintainability.Chores
golang.org/x/expmodule as indirect.Footnotes
https://go.dev/doc/go1.22#math_rand_v2 ↩
https://go.dev/doc/go1.21#slices ↩
https://go.dev/doc/go1.21#slog ↩