Add new get-matches command for direct tournament match access - #3
Conversation
📝 WalkthroughWalkthroughAdds a new "get-matches" CLI command that fetches matches from an external API, maps API responses to domain models, supports multiple in-memory filters (status, team, match type), and outputs results in table, JSON, or YAML formats. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Cmd as GetMatchesCmd
participant HTTP as HTTP Client
participant API as External API
participant Mapper as Mapper
participant Filter as Filter Logic
participant Formatter as Output Formatter
participant Stdout as stdout
User->>Cmd: Run(ctx)
Cmd->>HTTP: GET /matches (with timeout)
HTTP->>API: request
API-->>HTTP: MatchResponse[]
HTTP-->>Cmd: response
Cmd->>Mapper: ToDomainMatchesFromResponse(response)
Mapper-->>Cmd: []domain.Match
Cmd->>Filter: applyFilters(matches)
Filter-->>Cmd: filtered matches
Cmd->>Formatter: Format(stdout, filtered matches)
Formatter->>Stdout: write output
Stdout-->>User: display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Summary of ChangesHello @mgranderath, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI's functionality by introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new get-matches command, which is a great addition for directly accessing match data. The implementation is well-structured, with clear separation between the command, API models, mappers, and output formatters. The test coverage is also comprehensive.
I've identified a significant bug in the match status inference logic that could misclassify completed matches. I've also included a couple of suggestions to improve code modularity and reduce duplication. Overall, this is a solid contribution.
| func inferMatchStatus(maps []blast.MatchResponseMap) (isCompleted bool, isLive bool) { | ||
| if len(maps) == 0 { | ||
| return false, false | ||
| } | ||
|
|
||
| hasStarted := false | ||
| allEnded := true | ||
|
|
||
| for _, m := range maps { | ||
| if m.StartedAt != "" { | ||
| hasStarted = true | ||
| if m.EndedAt == "" { | ||
| allEnded = false | ||
| } | ||
| } else { | ||
| // Map hasn't started, so match isn't complete yet | ||
| allEnded = false | ||
| } | ||
| } | ||
|
|
||
| if !hasStarted { | ||
| return false, false // Upcoming | ||
| } | ||
|
|
||
| if allEnded { | ||
| return true, false // Completed | ||
| } | ||
|
|
||
| return false, true // Live | ||
| } |
There was a problem hiding this comment.
The current implementation of inferMatchStatus incorrectly assumes that all possible maps in a series must be played for a match to be considered complete. This will cause matches that finish early (e.g., a 3-0 in a Best-of-5) to be misclassified as 'Live'. The logic should only consider maps that have actually started to determine completion status. Additionally, it would be beneficial to add a test case for this scenario to prevent future regressions.
func inferMatchStatus(maps []blast.MatchResponseMap) (isCompleted bool, isLive bool) {
if len(maps) == 0 {
return false, false
}
hasStarted := false
allStartedHaveEnded := true
for _, m := range maps {
if m.StartedAt != "" {
hasStarted = true
if m.EndedAt == "" {
allStartedHaveEnded = false
}
}
}
if !hasStarted {
return false, false // Upcoming
}
if allStartedHaveEnded {
return true, false // Completed
}
return false, true // Live
}| func (g *GetMatchesCmd) Run(ctx *Context) error { | ||
| // Validate conflicting filters | ||
| filterCount := 0 | ||
| if g.CompletedOnly { | ||
| filterCount++ | ||
| } | ||
| if g.LiveOnly { | ||
| filterCount++ | ||
| } | ||
| if g.UpcomingOnly { | ||
| filterCount++ | ||
| } | ||
| if filterCount > 1 { | ||
| return fmt.Errorf("cannot use multiple status filters together (completed-only, live-only, upcoming-only are mutually exclusive)") | ||
| } | ||
|
|
||
| url := fmt.Sprintf("https://api.blast.tv/v2/games/rl/tournaments/%s/matches", g.TournamentID) | ||
|
|
||
| client := &http.Client{ | ||
| Timeout: 10 * time.Second, | ||
| } | ||
|
|
||
| req, err := http.NewRequest("GET", url, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| req.Header.Set("Accept", "application/json") | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to make request: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode == http.StatusNotFound { | ||
| return fmt.Errorf("tournament not found: %s", g.TournamentID) | ||
| } | ||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("unexpected status code: %d", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response body: %w", err) | ||
| } | ||
|
|
||
| var apiMatches []blast.MatchResponse | ||
| if err := json.Unmarshal(body, &apiMatches); err != nil { | ||
| return fmt.Errorf("failed to parse JSON: %w", err) | ||
| } | ||
|
|
||
| // Map API response to domain model | ||
| matches, err := mapper.ToDomainMatchesFromResponse(apiMatches) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to map matches: %w", err) | ||
| } | ||
|
|
||
| // Apply filters | ||
| matches = g.applyFilters(matches) | ||
|
|
||
| // Get the appropriate formatter | ||
| formatter, err := output.GetMatchesFormatter(g.Output) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get formatter: %w", err) | ||
| } | ||
|
|
||
| // Output using the selected formatter | ||
| if err := formatter.Format(os.Stdout, matches); err != nil { | ||
| return fmt.Errorf("failed to format output: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The Run function is responsible for a wide range of tasks, including filter validation, HTTP requests, response parsing, data mapping, filtering, and output formatting. To improve modularity and make the code easier to read and test, consider extracting the data fetching and parsing logic (lines 77-111) into a separate unexported function, such as fetchAndParseMatches(). This would allow Run to act as a higher-level coordinator, improving separation of concerns.
| // Handle empty time strings for fields that may not be set | ||
| var actualStart time.Time | ||
| if api.StartedAt != "" { | ||
| parsedTime, err := time.Parse(timeFormat, api.StartedAt) | ||
| if err != nil { | ||
| return domain.MatchMap{}, fmt.Errorf("failed to parse started at time: %w", err) | ||
| } | ||
| actualStart = parsedTime | ||
| } | ||
|
|
||
| var matchEnded time.Time | ||
| if api.EndedAt != "" { | ||
| parsedTime, err := time.Parse(timeFormat, api.EndedAt) | ||
| if err != nil { | ||
| return domain.MatchMap{}, fmt.Errorf("failed to parse ended at time: %w", err) | ||
| } | ||
| matchEnded = parsedTime | ||
| } |
There was a problem hiding this comment.
The code for parsing optional timestamp strings (StartedAt and EndedAt) is duplicated. To improve maintainability and reduce redundancy, this logic can be extracted into a helper function.
For example, you could add a helper function like this and use it as shown in the suggestion:
func parseOptionalTime(timeStr, fieldName string) (time.Time, error) {
if timeStr == "" {
return time.Time{}, nil
}
t, err := time.Parse(timeFormat, timeStr)
if err != nil {
return time.Time{}, fmt.Errorf("failed to parse %s time: %w", fieldName, err)
}
return t, nil
}| // Handle empty time strings for fields that may not be set | |
| var actualStart time.Time | |
| if api.StartedAt != "" { | |
| parsedTime, err := time.Parse(timeFormat, api.StartedAt) | |
| if err != nil { | |
| return domain.MatchMap{}, fmt.Errorf("failed to parse started at time: %w", err) | |
| } | |
| actualStart = parsedTime | |
| } | |
| var matchEnded time.Time | |
| if api.EndedAt != "" { | |
| parsedTime, err := time.Parse(timeFormat, api.EndedAt) | |
| if err != nil { | |
| return domain.MatchMap{}, fmt.Errorf("failed to parse ended at time: %w", err) | |
| } | |
| matchEnded = parsedTime | |
| } | |
| // Handle empty time strings for fields that may not be set | |
| actualStart, err := parseOptionalTime(api.StartedAt, "started at") | |
| if err != nil { | |
| return domain.MatchMap{}, err | |
| } | |
| matchEnded, err := parseOptionalTime(api.EndedAt, "ended at") | |
| if err != nil { | |
| return domain.MatchMap{}, err | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/output/matches_yaml_test.go`:
- Around line 120-129: The test TestMatchesYAMLFormatter_FormatEmpty currently
accepts either "[]" or "" for an empty slice which can hide write failures;
update the expectation to require the YAML-encoded empty array only. Change the
assertion in TestMatchesYAMLFormatter_FormatEmpty to call formatter.Format(&buf,
[]domain.Match{}) and then assert that strings.TrimSpace(buf.String()) == "[]",
removing the allowance for an empty string so any failure to write is detected;
reference the MatchesYAMLFormatter.Format method and the
TestMatchesYAMLFormatter_FormatEmpty test where the buffer contents are checked.
- Around line 108-115: The test currently loops over lines (variable lines from
output) but never fails if the 2-space indentation is missing; add a boolean
flag (e.g., foundTwoSpaceIndent) initialized false, set it true inside the loop
when the condition if strings.HasPrefix(line, " ") && !strings.HasPrefix(line,
" ") is met (then break), and after the loop assert the flag is true (e.g.,
using require.True/ assert.True with the test T) so the test actually validates
that a 2-space-indented line was present.
In `@internal/output/matches_yaml.go`:
- Around line 13-17: In MatchesYAMLFormatter.Format, change the deferred call to
yaml.Encoder.Close() so its error is captured and returned: don't just defer
encoder.Close() silently — call encoder.Encode(matches), capture its error, then
call encoder.Close() and if Close returns a non-nil error prefer/aggregate it
(or return it if Encode succeeded) so buffered write errors aren't swallowed;
update logic around yaml.NewEncoder, encoder.SetIndent, encoder.Encode and
encoder.Close to ensure both Encode and Close errors are handled and a non-nil
error is returned to the caller.
🧹 Nitpick comments (2)
internal/mapper/match_test.go (1)
368-442: Consider adding a test case for invalidEndedAttime.The
TestToDomainMatchMapFromResponsecovers invalidScheduledAtand invalidStartedAt, but doesn't test the error path for an invalidEndedAtvalue. This would improve coverage of the parsing logic at lines 88-94 inmatch.go.📝 Suggested test case to add
{ name: "invalid ended time", api: blast.MatchResponseMap{ ID: "map-5", Name: "Invalid", ScheduledAt: "2026-01-15T12:00:00.000Z", StartedAt: "2026-01-15T12:05:00.000Z", EndedAt: "invalid", }, expectError: true, },internal/cmd/get_matches.go (1)
77-101: Consider using request context for cancellation support.The HTTP request is created without a context, which means it cannot be cancelled if the CLI receives a termination signal during a long-running request. While the 10-second timeout provides a safety net, using
http.NewRequestWithContextwith a cancellable context would improve responsiveness to user interrupts.♻️ Suggested improvement
+import "context" func (g *GetMatchesCmd) Run(ctx *Context) error { // ...validation... url := fmt.Sprintf("https://api.blast.tv/v2/games/rl/tournaments/%s/matches", g.TournamentID) client := &http.Client{ Timeout: 10 * time.Second, } - req, err := http.NewRequest("GET", url, nil) + reqCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, "GET", url, nil) if err != nil { return fmt.Errorf("failed to create request: %w", err) }
- Fix critical bug in inferMatchStatus: unplayed maps no longer prevent completed matches from being correctly classified - Fix YAML encoder to properly handle Close() errors - Fix YAML tests: stricter empty check and proper indentation validation - Add test cases for invalid EndedAt time and early-ending matches (BO7 4-0)
1 participant
Summary
This PR adds a new
get-matchescommand that fetches matches directly from the Blast API/matchesendpoint, providing a simpler alternative to the existingget-bracketscommand which extracts matches from bracket structures.Changes
New Features
get-matchescommand: Fetches all matches for a tournament directly from the APIget-brackets:--completed-only,--live-only,--upcoming-only,--team,--match-typetable(default),json,yamlNew Files Created
internal/api/blast/match_models.go- API types for matches endpointinternal/mapper/match.go- Maps API response to domain models with status inferenceinternal/output/matches_formatter.go- Formatter interface and registryinternal/output/matches_table.go- Table formatter for matchesinternal/output/matches_json.go- JSON formatterinternal/output/matches_yaml.go- YAML formatterinternal/cmd/get_matches.go- Command implementationTest Coverage
All new functionality includes comprehensive tests:
internal/mapper/match_test.go(93.4% coverage)internal/output/*_test.go(76.1% coverage)internal/cmd/get_matches_test.go(81.6% coverage)Modified Files
internal/cmd/root.go- Addedget-matchescommand to CLIUsage Examples
Implementation Notes
Status Inference: Since the matches API doesn't provide explicit
isLive/isCompletedbooleans, status is inferred from map timestamps:Reusable Components: The implementation reuses existing domain models (
domain.Match,domain.MatchTeam,domain.MatchMap) and follows the same patterns asget-brackets.Testing
All tests pass with good coverage:
go test ./... ok github.com/mgranderath/rlcs-cli/internal/cmd 0.480s coverage: 81.6% of statements ok github.com/mgranderath/rlcs-cli/internal/mapper 0.335s coverage: 93.4% of statements ok github.com/mgranderath/rlcs-cli/internal/output 0.489s coverage: 76.1% of statementsSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.