Output and consider Go toolchain version, too #156
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThe recent code changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CheckCmd
participant GoUtil
participant UpdateCmd
Note over GoUtil,UpdateCmd: Updated Go toolchain available
User->>CheckCmd: Execute check command
CheckCmd->>GoUtil: Check latest version and build toolchain
GoUtil->>CheckCmd: Return version and Go toolchain status
CheckCmd->>User: Inform if binary needs updating or toolchain refresh
User->>UpdateCmd: Execute update command
UpdateCmd->>GoUtil: Use current Go toolchain to update
GoUtil->>UpdateCmd: Return updated status
UpdateCmd->>User: Confirm update
Assessment against linked issues
Poem
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 (
|
|
@nao1215 feel free to push to this branch, I'll probably get to it eventually to finish this but it might take a while. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
cmd/check.go (1)
Line range hint
143-143: Avoid using magic numbers in the code to enhance maintainability.Consider defining a constant for the magic number
11used in the indentation:- strings.Repeat(" ", 11) + + const indentSpace = 11 + strings.Repeat(" ", indentSpace) +internal/goutil/goutil_test.go (2)
Line range hint
423-424: Ensure to handle potential errors from environment variable setting.- t.Setenv("GOBIN", "") - t.Setenv("GOPATH", "") + if err := t.Setenv("GOBIN", ""); err != nil { + t.Fatal("Failed to set GOBIN:", err) + } + if err := t.Setenv("GOPATH", ""); err != nil { + t.Fatal("Failed to set GOPATH:", err) + }Handling errors from
t.Setenvensures that the test setup is correctly configured and prevents false negatives in the test environment.
Line range hint
166-166: Consider defining a constant for repeated strings likeecho.+ const mockGoCommand = "echo" - goExe = "echo" + goExe = mockGoCommandUsing a constant for repeated string values like
echoused in multiple test cases enhances maintainability and readability by centralizing the change to one location.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- cmd/check.go (2 hunks)
- cmd/update.go (1 hunks)
- internal/goutil/examples_test.go (1 hunks)
- internal/goutil/goutil.go (6 hunks)
- internal/goutil/goutil_test.go (4 hunks)
Additional context used
golangci-lint
cmd/check.go
143-143: Magic number: 11, in detected (gomnd)
internal/goutil/examples_test.go
142-142: tests: ExampleInstall refers to unknown identifier: Install (govet)
163-163: tests: ExampleIsAlreadyUpToDate refers to unknown identifier: IsAlreadyUpToDate (govet)
internal/goutil/goutil.go
336-336: string
unknownhas 5 occurrences, make it a constant (goconst)
160-160: ifElseChain: rewrite if-else to switch statement (gocritic)
185-185: ifElseChain: rewrite if-else to switch statement (gocritic)
23-23: goExe is a global variable (gochecknoglobals)
25-25: keyGoBin is a global variable (gochecknoglobals)
27-27: keyGoPath is a global variable (gochecknoglobals)
29-29: osMkdirTemp is a global variable (gochecknoglobals)
internal/goutil/goutil_test.go
423-423: Error return value of
os.Setenvis not checked (errcheck)
424-424: Error return value of
os.Setenvis not checked (errcheck)
166-166: string
echohas 3 occurrences, make it a constant (goconst)
Additional comments not posted (7)
cmd/check.go (1)
20-23: Updated command description to reflect the enhanced functionality of checking both the binary version and the Go toolchain version.cmd/update.go (1)
30-31: Updated command description to clarify that updates will use the current installed Go toolchain.internal/goutil/examples_test.go (1)
164-182: The test example has been updated to include checks for both the package version and the Go version.internal/goutil/goutil.go (1)
52-53: AddedGoVersionfield to thePackagestruct to store the Go toolchain version.internal/goutil/goutil_test.go (3)
286-337: The updatedTestIsAlreadyUpToDate_goldenfunction effectively captures the new requirement to consider both the package version and the Go toolchain version. The test cases are comprehensive, covering regular, irregular, and compatibility scenarios. This ensures robustness in the functionality to check if a package is up-to-date considering both the software and the Go version.
560-563: The addition ofGoVersionto thePackagestruct in these test functions (TestPackage_CurrentToLatestStr_not_up_to_dateandTestPackage_VersionCheckResultStr_not_up_to_date) is a welcome change, aligning with the new feature requirements. This modification allows the functions to check both the package version and the Go toolchain version, enhancing the utility and accuracy of the version checks.Also applies to: 584-587
599-620: The functionTestPackage_VersionCheckResultStr_go_not_up_to_datecorrectly implements the new logic to compare the current Go version with the installed version. This is crucial for ensuring that the software is running on the latest Go toolchain, which can include important performance improvements and bug fixes.
| @@ -92,7 +93,7 @@ func doCheck(pkgs []goutil.Package, cpus int) int { | |||
| err = fmt.Errorf(" %s %w", p.Name, err) | |||
| } | |||
| p.Version.Latest = latestVer | |||
| if !goutil.IsAlreadyUpToDate(*p.Version) { | |||
| if !p.IsAlreadyUpToDate() { | |||
There was a problem hiding this comment.
Ensure the mutex lock is held for the shortest time necessary to avoid potential performance bottlenecks.
Consider moving the mu.Unlock() call immediately after the append operation to minimize the critical section:
mu.Lock()
needUpdatePkgs = append(needUpdatePkgs, p)
mu.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !p.IsAlreadyUpToDate() { | |
| if !p.IsAlreadyUpToDate() { | |
| mu.Lock() | |
| needUpdatePkgs = append(needUpdatePkgs, p) | |
| mu.Unlock() | |
| } |
| if p.IsAlreadyUpToDate() { | ||
| return "Already up-to-date: " + color.GreenString(p.Version.Latest) + " / " + color.GreenString(p.GoVersion.Current) | ||
| } | ||
| return color.GreenString(p.Version.Current) + " to " + color.YellowString(p.Version.Latest) | ||
| var ret string | ||
| if p.Version.Current != p.Version.Latest { | ||
| ret += color.GreenString(p.Version.Current) + " to " + color.YellowString(p.Version.Latest) | ||
| } | ||
| if p.GoVersion.Current != p.GoVersion.Latest { | ||
| if len(ret) != 0 { | ||
| ret += ", " | ||
| } | ||
| ret += color.GreenString(p.GoVersion.Current) + " to " + color.YellowString(p.GoVersion.Latest) | ||
| } | ||
| return ret |
There was a problem hiding this comment.
Enhanced the string representation functions to include both package and Go toolchain versions.
However, consider simplifying the logic to improve readability and maintainability.
Refactor the repeated conditional logic into a helper function:
func formatVersionChange(current, latest string) string {
if current == latest {
return color.GreenString(current)
}
return "current: " + color.GreenString(current) + ", latest: " + color.YellowString(latest)
}Also applies to: 97-123
Some tests stopped passing due to the addition of a specification to check the Go version. The tests that do not pass were examples for developers, so they have been cleanly removed.
|
@scop |
|
Thanks! |
Closes #154
Summary by CodeRabbit
New Features
checkcommand to verify both the latest version and the build toolchain of the installed binary.updatecommand to utilize the current Go toolchain for updates.Bug Fixes
importcommand.Tests
Dependencies
github.com/shogo82148/pointeras a new dependency.Internal Improvements