Conversation
WalkthroughThe recent updates streamline the handling of GOPATH and GOBIN within a Go project. Notably, unnecessary imports and functions related to environment variable checks have been removed, simplifying the codebase. A significant enhancement includes a new method for dynamically retrieving the GOPATH, ensuring greater flexibility and reliability in environments where this path is not explicitly set. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- cmd/remove_test.go (2 hunks)
- internal/goutil/goutil.go (1 hunks)
- internal/goutil/goutil_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- cmd/remove_test.go
Additional comments: 2
internal/goutil/goutil.go (1)
- 247-250: The addition of
exec.Commandto dynamically retrieve theGOPATHis a welcome improvement for flexibility. However, consider handling the case where thegocommand is not available or fails unexpectedly. It might be beneficial to log the error or provide a fallback mechanism.internal/goutil/goutil_test.go (1)
- 191-196: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of tests related to
GOPATHhandling suggests a shift in testing strategy. Ensure that the new approach for dynamically retrievingGOPATHis adequately tested, possibly by mockingexec.Commandto simulate different scenarios.
|
@rkscv |
|
Do you mind if I delete these tests? Or any suggestions? |
|
@rkscv func TestGetPackageVersion_getting_error_from_gobin(t *testing.T) {
// Set env variable to temporary value and defer restore on Cleanup
t.Setenv("GOBIN", "")
t.Setenv("GOPATH", "")
// Backup and defer restore
oldKeyGoPath := keyGoPath
defer func() {
keyGoPath = oldKeyGoPath
}()
// Mock the value
keyGoPath = t.Name()
// Setting GOBIN, GOPATH and build.Default.GOPATH to empty string
// should be an error internally and return "unknown" as a version.
got := GetPackageVersion(".")
want := "unknown"
if want != got {
t.Errorf("GetPackageVersion() should return %v. got: %v", want, got)
}
} |
|
Thanks, it looks fine. But |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- cmd/remove_test.go (2 hunks)
- internal/goutil/goutil.go (1 hunks)
- internal/goutil/goutil_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- cmd/remove_test.go
- internal/goutil/goutil.go
- internal/goutil/goutil_test.go
|
@rkscv |
ghost
deleted the
1 participant
https://github.com/golang/go/blob/35b1a146d9febca70db87dc4e1f8bac33de857bb/src/go/build/build.go#L289-L306
build.Default.GOPATHis hard-coded~/gowhile I use a customGOPATHin~/.config/go/envmanaged bygo env.I don't tend to test
goPathbecause~/.config/go/envcould be read or written by other programs during the test.Summary by CodeRabbit