refactor: share offline player data loading helper#85
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors offline player analysis loading by extracting shared logic into Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
cmd/cr-api/player_analysis_runtime_test.go (2)
213-225: Strengthen the error-wrapping assertion.The current check (
got == "boom") only verifies the message is not the bare loader error. It would still pass if the helper returned, say, an unrelated wrapped error like"foo: bar". Since the helper uses%w, prefererrors.Isto verify proper unwrapping, and optionally assert the contextual prefix.♻️ Suggested stronger assertion
+ sentinel := errors.New("boom") loader := &fakeOfflineAnalysisLoader{ - loadLatestErr: errors.New("boom"), + loadLatestErr: sentinel, } _, err := loadOfflineDeckPlayerData(loader, "#POFFLINE", "", "", "testdata") if err == nil { t.Fatal("expected error") } - if got := err.Error(); got == "boom" { - t.Fatalf("expected wrapped error, got %q", got) + if !errors.Is(err, sentinel) { + t.Fatalf("expected wrapped sentinel error, got %v", err) + } + if !strings.Contains(err.Error(), "#POFFLINE") { + t.Fatalf("expected error to include player tag, got %q", err.Error()) }(Add
"strings"to the imports.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cr-api/player_analysis_runtime_test.go` around lines 213 - 225, The test TestLoadOfflineDeckPlayerDataWrapsLoadErrors should assert that the loader error is properly wrapped using errors.Is rather than only checking the error message; update the test to import "errors" (and add "strings" if you also assert the contextual prefix) and replace the current string equality check with errors.Is(err, loader.loadLatestErr) to confirm unwrapping, and optionally assert that err.Error() has the expected contextual prefix (e.g., with strings.HasPrefix) to verify the helper added context.
22-50: Nice test harness — good coverage of the new helper.The
fakeOfflineAnalysisLoadercleanly captures call counts, inputs, and configurable errors, and the three tests exercise the meaningful branches (default dir + tag fallback, explicit file with provided identity, and error propagation).One minor gap worth considering: there's no test for the case where
analysisDiris explicitly provided (non-empty) — i.e., thatloadOfflineDeckPlayerDatadoes not apply thefilepath.Join(dataDir, "analysis")default and uses the caller-supplied directory verbatim. That branch is exercised byloadPlayerDataOfflinein production but not directly covered here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cr-api/player_analysis_runtime_test.go` around lines 22 - 50, Add a unit test that verifies the explicit analysisDir path is used (i.e., loadOfflineDeckPlayerData does not fall back to filepath.Join(dataDir, "analysis") when a non-empty analysisDir is provided): instantiate fakeOfflineAnalysisLoader, call loadOfflineDeckPlayerData (or the wrapper exercised by tests) with a non-empty analysisDir value, then assert fakeOfflineAnalysisLoader.latestDir equals that exact provided directory and that latestCalls was incremented; reference fakeOfflineAnalysisLoader, LoadLatestAnalysis, and loadOfflineDeckPlayerData when implementing the test.cmd/cr-api/player_analysis_runtime.go (1)
38-73: Consider guarding against(nil, nil)returns from the loader.Lines 58 and 63 dereference
loadedAnalysisimmediately after a successful load. The realdeck.Builderimplementations probably never return(nil, nil), but theofflineAnalysisLoaderinterface doesn't enforce that contract — a misbehaving (or test-stub) loader returning a nil analysis with no error would panic here rather than producing a clean error. A small nil check would make this helper robust to interface misuse.🛡️ Suggested defensive guard
if analysisFile != "" { loadedAnalysis, err = loader.LoadAnalysis(analysisFile) if err != nil { return nil, fmt.Errorf("failed to load analysis file %s: %w", analysisFile, err) } } else { loadedAnalysis, err = loader.LoadLatestAnalysis(tag, analysisDir) if err != nil { return nil, fmt.Errorf("failed to load analysis for player %s from %s: %w", tag, analysisDir, err) } } + if loadedAnalysis == nil { + return nil, fmt.Errorf("loader returned nil analysis for player %s", tag) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cr-api/player_analysis_runtime.go` around lines 38 - 73, The helper loadOfflineDeckPlayerData should guard against a loader returning (nil, nil): after calling loader.LoadAnalysis or loader.LoadLatestAnalysis check if loadedAnalysis == nil and return a clear error (e.g. fmt.Errorf("loader returned nil CardAnalysis for tag %s, file/dir %s", tag, analysisFileOrDir)) instead of dereferencing it; update the function to validate loadedAnalysis before using loadedAnalysis.PlayerName/PlayerTag so offlineAnalysisLoader implementations (and test stubs) that return nil don't cause a panic.cmd/cr-api/deck_compare_suite_runtime.go (1)
122-137: LGTM — clean delegation, behavior preserved.The refactor correctly forwards to
loadOfflineDeckPlayerData(builder, tag, "", "", dataDir), which preserves the priorfilepath.Join(dataDir, "analysis")defaulting and thetag-based fallback for emptyPlayerName/PlayerTag.Optional follow-up:
suitePlayerData(lines 95–99) andofflineDeckPlayerData(inplayer_analysis_runtime.go) now have identical fields. Consider consolidating to a single type or havingsuitePlayerDataembed/aliasofflineDeckPlayerDatato remove the field-by-field copy at lines 132–136.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cr-api/deck_compare_suite_runtime.go` around lines 122 - 137, suitePlayerData duplicates offlineDeckPlayerData fields and loadSuitePlayerDataFromAnalysis performs a field-by-field copy; change to reuse the existing type by either making suitePlayerData an alias or embedding offlineDeckPlayerData (or returning *offlineDeckPlayerData directly) so you can return the loaded struct without manual copying; update the suitePlayerData type declaration (or function return type) and adjust any callers of loadSuitePlayerDataFromAnalysis or references to suitePlayerData/offlineDeckPlayerData to match the new single combined type (refer to suitePlayerData, offlineDeckPlayerData, and loadSuitePlayerDataFromAnalysis for locations to change).cmd/cr-api/deck_builder_config_runtime.go (1)
255-265: Default analysis-dir resolution is now duplicated.The
filepath.Join(dataDir, "analysis")fallback on lines 259–262 mirrors the same defaulting insideloadOfflineDeckPlayerData(player_analysis_runtime.golines 39–41). If the default location ever changes, both call sites need updating, defeating part of the purpose of the helper.Consider returning the resolved source/path from
loadOfflineDeckPlayerData(e.g., add aSource stringfield onofflineDeckPlayerData) so the verbose logging can use it without re-deriving the default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cr-api/deck_builder_config_runtime.go` around lines 255 - 265, The code duplicates the default analysis-dir resolution; modify loadOfflineDeckPlayerData so it returns the resolved analysis path by adding a Source string field to the offlineDeckPlayerData (or returning the source as a second return value) and set it to the actual directory used (falling back to filepath.Join(dataDir, "analysis") inside that function); then update callers (including where offlineDeckPlayerData is consumed in the verbose block in deck_builder_config_runtime.go) to use offlineDeckPlayerData.Source for the "Loaded analysis" log instead of recomputing the default, and update any function signatures/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/cr-api/deck_builder_config_runtime.go`:
- Around line 255-265: The code duplicates the default analysis-dir resolution;
modify loadOfflineDeckPlayerData so it returns the resolved analysis path by
adding a Source string field to the offlineDeckPlayerData (or returning the
source as a second return value) and set it to the actual directory used
(falling back to filepath.Join(dataDir, "analysis") inside that function); then
update callers (including where offlineDeckPlayerData is consumed in the verbose
block in deck_builder_config_runtime.go) to use offlineDeckPlayerData.Source for
the "Loaded analysis" log instead of recomputing the default, and update any
function signatures/usages accordingly.
In `@cmd/cr-api/deck_compare_suite_runtime.go`:
- Around line 122-137: suitePlayerData duplicates offlineDeckPlayerData fields
and loadSuitePlayerDataFromAnalysis performs a field-by-field copy; change to
reuse the existing type by either making suitePlayerData an alias or embedding
offlineDeckPlayerData (or returning *offlineDeckPlayerData directly) so you can
return the loaded struct without manual copying; update the suitePlayerData type
declaration (or function return type) and adjust any callers of
loadSuitePlayerDataFromAnalysis or references to
suitePlayerData/offlineDeckPlayerData to match the new single combined type
(refer to suitePlayerData, offlineDeckPlayerData, and
loadSuitePlayerDataFromAnalysis for locations to change).
In `@cmd/cr-api/player_analysis_runtime_test.go`:
- Around line 213-225: The test TestLoadOfflineDeckPlayerDataWrapsLoadErrors
should assert that the loader error is properly wrapped using errors.Is rather
than only checking the error message; update the test to import "errors" (and
add "strings" if you also assert the contextual prefix) and replace the current
string equality check with errors.Is(err, loader.loadLatestErr) to confirm
unwrapping, and optionally assert that err.Error() has the expected contextual
prefix (e.g., with strings.HasPrefix) to verify the helper added context.
- Around line 22-50: Add a unit test that verifies the explicit analysisDir path
is used (i.e., loadOfflineDeckPlayerData does not fall back to
filepath.Join(dataDir, "analysis") when a non-empty analysisDir is provided):
instantiate fakeOfflineAnalysisLoader, call loadOfflineDeckPlayerData (or the
wrapper exercised by tests) with a non-empty analysisDir value, then assert
fakeOfflineAnalysisLoader.latestDir equals that exact provided directory and
that latestCalls was incremented; reference fakeOfflineAnalysisLoader,
LoadLatestAnalysis, and loadOfflineDeckPlayerData when implementing the test.
In `@cmd/cr-api/player_analysis_runtime.go`:
- Around line 38-73: The helper loadOfflineDeckPlayerData should guard against a
loader returning (nil, nil): after calling loader.LoadAnalysis or
loader.LoadLatestAnalysis check if loadedAnalysis == nil and return a clear
error (e.g. fmt.Errorf("loader returned nil CardAnalysis for tag %s, file/dir
%s", tag, analysisFileOrDir)) instead of dereferencing it; update the function
to validate loadedAnalysis before using loadedAnalysis.PlayerName/PlayerTag so
offlineAnalysisLoader implementations (and test stubs) that return nil don't
cause a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7666fbc6-2db0-4577-9461-212c9bf3550f
📒 Files selected for processing (4)
cmd/cr-api/deck_builder_config_runtime.gocmd/cr-api/deck_compare_suite_runtime.gocmd/cr-api/player_analysis_runtime.gocmd/cr-api/player_analysis_runtime_test.go
1 participant
Summary
loadOfflineDeckPlayerDatahelper for offline analysis loading behaviorloadPlayerDataOfflineandloadSuitePlayerDataFromAnalysisTesting
GOCACHE=/tmp/go-build-cache GOMODCACHE=/tmp/go-mod-cache go test ./cmd/cr-api -run 'TestLoadOnlinePlayerAnalysisPreservesEvolutionLevels|TestLoadSuitePlayerDataFromAPIUsesSharedDeckAnalysis|TestLoadOfflineDeckPlayerDataDefaultsDirAndFallbacks|TestLoadOfflineDeckPlayerDataUsesExplicitFile|TestLoadOfflineDeckPlayerDataWrapsLoadErrors'Summary by CodeRabbit
Refactor
Tests