Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps project version v1.8.10 → v1.8.11 across build/config files, adds utils.GenerateHelpMessage and refactors many CLI commands to use it, changes SearchCommand() to return (string, error) and updates its tests, and modifies dbt/project and card_data pipeline assets/sources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
This was
linked to
issues
Feb 24, 2026
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/utils/help.go (1)
23-26: Consider usingstrings.BuilderforflagsListinstead of+=concatenation in a loop.Each
+=allocates a new string. Astrings.Builderwrites in-place and avoids repeated allocations.♻️ Proposed refactor
- flagsList := "" - for _, f := range cfg.Flags { - flagsList += fmt.Sprintf("\n\t%-30s %s", f.Short+", "+f.Long, f.Description) - } + var flagsBuilder strings.Builder + for _, f := range cfg.Flags { + fmt.Fprintf(&flagsBuilder, "\n\t%-30s %s", f.Short+", "+f.Long, f.Description) + } + flagsList := flagsBuilder.String()Also add
"strings"to the import block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/utils/help.go` around lines 23 - 26, Replace repeated string concatenation for flagsList with a strings.Builder: inside the loop over cfg.Flags (the block that declares flagsList and iterates "for _, f := range cfg.Flags"), create a strings.Builder, use builder.WriteString / fmt.Fprintf(&builder, ...) to append each formatted line instead of flagsList +=, then assign flagsList = builder.String() after the loop; also add "strings" to the import block. This updates the code that builds flag lines (the flagsList variable and the loop over cfg.Flags) to avoid repeated allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/pokemon/pokemon.go`:
- Line 37: Update the flag description for the moves flag (the struct literal
with Short: "-m" and Long: "--moves") to use the correct accented form
"Pokémon's" instead of "Pokemon's" so it matches the other flag descriptions;
locate the flag entry where Description is "Prints the Pokemon's learnable
moves." and change the string to "Prints the Pokémon's learnable moves.".
In `@cmd/search/search_test.go`:
- Around line 45-49: Replace the non-fatal assertions with fatal ones in the
test: where the table test checks tt.expectedError use require.Error(t, err) and
require.NoError(t, err) instead of assert.Error/assert.NoError so the test exits
immediately on failure (preventing the later strings.Contains(check on
cleanOutput) from running on invalid state); also add
"github.com/stretchr/testify/require" to the test imports. Target the assertions
in cmd/search/search_test.go that reference tt.expectedError and err and the
later cleanOutput usage/comments.
In `@cmd/utils/help.go`:
- Line 34: Replace the avoidable fmt.Sprintf call in help.go: instead of using
fmt.Sprintf("%s\n\n", cfg.Description) (the call in the code that references
cfg.Description), build the string via simple concatenation so no formatting
function is used; update the expression returning the description to concatenate
cfg.Description with the two newline characters directly to satisfy the
perfsprint rule.
In `@testdata/pokemon_help.golden`:
- Line 13: Update the help text for the moves flag to use the accented form
"Pokémon" for consistency: find the line containing the moves flag description
(the "-m, --moves" help entry) and replace "Pokemon's" with "Pokémon's" so the
line reads "Prints the Pokémon's learnable moves." to match the rest of the help
output.
---
Nitpick comments:
In `@cmd/utils/help.go`:
- Around line 23-26: Replace repeated string concatenation for flagsList with a
strings.Builder: inside the loop over cfg.Flags (the block that declares
flagsList and iterates "for _, f := range cfg.Flags"), create a strings.Builder,
use builder.WriteString / fmt.Fprintf(&builder, ...) to append each formatted
line instead of flagsList +=, then assign flagsList = builder.String() after the
loop; also add "strings" to the import block. This updates the code that builds
flag lines (the flagsList variable and the loop over cfg.Flags) to avoid
repeated allocations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/ci.yml.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pyproject.tomlcli.gocmd/ability/ability.gocmd/berry/berry.gocmd/card/card.gocmd/item/item.gocmd/move/move.gocmd/natures/natures.gocmd/pokemon/pokemon.gocmd/search/search.gocmd/search/search_test.gocmd/speed/speed.gocmd/types/types.gocmd/utils/help.gocmd/utils/help_test.gonfpm.yamltestdata/ability_help.goldentestdata/item_help.goldentestdata/main_latest_flag.goldentestdata/move_help.goldentestdata/natures_help.goldentestdata/pokemon_help.goldentestdata/speed_help.goldentestdata/types_help.golden
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/search/search_test.go (1)
81-89:⚠️ Potential issue | 🟠 MajorReplace
assert.Errorwithrequire.Errorto comply with testifylint rules.The
testifylintlinter is enabled in.golangci.ymland enforced in the CI pipeline (.github/workflows/go_lint.yml). Therequire-errorrule requires usingrequire.Errorfor error assertions in tests, which will cause CI failures with the currentassert.Errorcall.Suggested change
- _, err := SearchCommand() - assert.Error(t, err, "SearchCommand should return error for invalid args") + _, err := SearchCommand() + require.Error(t, err, "SearchCommand should return error for invalid args")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/search/search_test.go` around lines 81 - 89, In TestSearchCommandValidationError (function TestSearchCommandValidationError) replace the assert.Error call with require.Error to satisfy the testifylint rule; update the test imports to import require from "github.com/stretchr/testify/require" if not already present and remove or keep assert depending on other uses in the file so the test fails fast on error checks (i.e., change assert.Error(t, err, ...) to require.Error(t, err, "...")).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/search/search_test.go`:
- Around line 81-89: In TestSearchCommandValidationError (function
TestSearchCommandValidationError) replace the assert.Error call with
require.Error to satisfy the testifylint rule; update the test imports to import
require from "github.com/stretchr/testify/require" if not already present and
remove or keep assert depending on other uses in the file so the test fails fast
on error checks (i.e., change assert.Error(t, err, ...) to require.Error(t, err,
"...")).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/pokemon/pokemon.gocmd/search/search_test.gocmd/utils/help.gotestdata/pokemon_help.golden
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/pokemon/pokemon.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/card/serieslist_test.go (1)
155-166:⚠️ Potential issue | 🟡 MinorUse
t.Fatalfon length mismatch before indexed assertions.On Line 155, the test records an error but still indexes
items[i]; if length is shorter, this can panic and hide the real failure.Suggested fix
- if len(items) != 4 { - t.Errorf("Expected 4 items, got %d", len(items)) - } + if len(items) != 4 { + t.Fatalf("Expected 4 items, got %d", len(items)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/card/serieslist_test.go` around lines 155 - 166, The length check for items uses t.Errorf which allows the test to continue and may cause a panic when indexing items[i]; change the length assertion in the test to stop execution on failure (use t.Fatalf or return after logging) so the subsequent loop over expectedSeries that accesses items[i] cannot index out of range; update the assertion that currently reads "Expected 4 items, got %d" to call t.Fatalf (or otherwise abort) before the indexed verification loop that uses items and expectedSeries.
🧹 Nitpick comments (4)
cmd/card/serieslist.go (1)
63-69: Consider a single source of truth for series definitions.
seriesIDMapanditemsduplicate the same series set. Generating items from one ordered series definition would reduce drift risk when adding/removing series.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/card/serieslist.go` around lines 63 - 69, seriesIDMap and the items slice in SeriesList duplicate the same series definitions; consolidate into one ordered source of truth (e.g., a single []string or []struct with ID/label) and generate both the seriesIDMap and the list.Item slice from that single definition. Update the SeriesList() function to iterate the canonical series slice to build items (using item(...)) and rebuild seriesIDMap from the same loop (or a small helper like buildSeriesIDMap), and remove the duplicate hard-coded entries so additions/removals only occur in the one canonical series definition.card_data/pipelines/poke_cli_dbt/models/sources.yml (1)
105-112: Add minimal column contracts for new staging tables.For
standings,tournaments, andcountry_codes, consider defining key columns to strengthen source documentation and downstream schema checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/poke_cli_dbt/models/sources.yml` around lines 105 - 112, Add minimal column contracts for the three new sources by adding a columns: section under each source definition (standings, tournaments, country_codes). For standings, declare key columns like tournament_id, player_id, standing_rank (or rank) and any date field; for tournaments, declare tournament_id, tournament_name, start_date (and end_date if present); for country_codes, declare country_code and country_name. Use the "columns:" YAML list with name and description entries for each column so downstream schema checks and docs pick them up.card_data/pipelines/defs/extract/tcgdex/extract_series.py (1)
34-34: Consider centralizing supported series IDs.The allowed IDs list on Line 34 is now another hardcoded series definition. Moving this to a shared constant (used by extract_sets/CLI too) will reduce future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/defs/extract/tcgdex/extract_series.py` at line 34, Replace the hardcoded series ID list ["me","sv","swsh","sm"] used in the list comprehension with a shared constant (e.g., SUPPORTED_SERIES_IDS) that you define in a central module; add the constant to a common place consumed by extract_series.py, extract_sets, and the CLI, import SUPPORTED_SERIES_IDS into extract_series.py and use it in the comprehension (the expression currently filtering by s.id in ["me","sv","swsh","sm"]), and update extract_sets and CLI to import the same constant so all code references the single source of truth.card_data/pipelines/defs/extract/tcgcsv/extract_pricing.py (1)
93-103: Add regression tests for SM card-number normalization.The new formatting path is important for key matching; add unit/integration coverage for values like numeric (
1/149), already padded, and alphanumeric (SM115) numbers.Also applies to: 182-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/defs/extract/tcgcsv/extract_pricing.py` around lines 93 - 103, Add regression tests for the card-number normalization to cover numeric, already-padded, and alphanumeric cases: create unit tests calling normalize_card_number with "1/149" (expect "001/149"), "10/149" (expect "010/149"), an already padded value like "001/149" (expect unchanged), and an alphanumeric like "SM115" (expect unchanged). Also add the same set of assertions for the related normalization logic referenced around lines 182-184 to ensure both code paths preserve alphanumeric values and correctly zero-pad numeric left/right parts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@card_data/pipelines/definitions.py`:
- Around line 7-11: The imports for create_standings_dataframe and
load_standings_data reference non-existent modules under defs.extract.limitless
and defs.load.limitless; either remove these imports from
card_data/pipelines/definitions.py or add the missing modules. To fix: if
standings functionality is required, create modules matching the import paths
and implement the functions create_standings_dataframe and load_standings_data;
otherwise delete the two import lines (or replace them with the correct existing
module names) so that definitions.py only imports valid modules.
In `@card_data/pipelines/defs/extract/tcgdex/extract_cards.py`:
- Around line 100-103: The serialized attacks JSON (flat["attacks_json"]) uses
the unfiltered list but the code then builds per-attack columns from a filtered
list (attacks), causing index drift; change the serialization to use the same
filtered list by computing the filtered attacks once (e.g., attacks = [atk for
atk in card.get("attacks", []) if atk.get("name")]) and then set
flat["attacks_json"] = json.dumps(attacks, ensure_ascii=False) so the JSON and
the attack_1_*/attack_N_* columns remain aligned.
In `@card_data/pipelines/defs/extract/tcgdex/extract_sets.py`:
- Line 27: Tests and mocks still expect only three series (me, sv, swsh) but you
added the SM endpoint in the series list in extract_sets.py; update
card_data/pipelines/tests/extract_sets_test.py to include the "sm" series in the
expected_series_ids (or whatever test variable asserts series membership),
update any mocked API responses/fixtures used by those tests to include an SM
entry, and adjust any assertions counting total series to expect four so the
tests match the new URL added ("https://api.tcgdex.net/v2/en/series/sm").
In `@card_data/pipelines/soda/checks_sets.yml`:
- Line 3: The row-count check in checks_sets.yml currently uses "row_count > 41"
which erroneously rejects datasets with exactly 41 rows; update the check
expression in that file (the row_count gate) to use "row_count >= 41" (or
equivalently "row_count > 40") so that 41 rows pass; locate the check expression
"row_count > 41" and replace it accordingly.
---
Outside diff comments:
In `@cmd/card/serieslist_test.go`:
- Around line 155-166: The length check for items uses t.Errorf which allows the
test to continue and may cause a panic when indexing items[i]; change the length
assertion in the test to stop execution on failure (use t.Fatalf or return after
logging) so the subsequent loop over expectedSeries that accesses items[i]
cannot index out of range; update the assertion that currently reads "Expected 4
items, got %d" to call t.Fatalf (or otherwise abort) before the indexed
verification loop that uses items and expectedSeries.
---
Nitpick comments:
In `@card_data/pipelines/defs/extract/tcgcsv/extract_pricing.py`:
- Around line 93-103: Add regression tests for the card-number normalization to
cover numeric, already-padded, and alphanumeric cases: create unit tests calling
normalize_card_number with "1/149" (expect "001/149"), "10/149" (expect
"010/149"), an already padded value like "001/149" (expect unchanged), and an
alphanumeric like "SM115" (expect unchanged). Also add the same set of
assertions for the related normalization logic referenced around lines 182-184
to ensure both code paths preserve alphanumeric values and correctly zero-pad
numeric left/right parts.
In `@card_data/pipelines/defs/extract/tcgdex/extract_series.py`:
- Line 34: Replace the hardcoded series ID list ["me","sv","swsh","sm"] used in
the list comprehension with a shared constant (e.g., SUPPORTED_SERIES_IDS) that
you define in a central module; add the constant to a common place consumed by
extract_series.py, extract_sets, and the CLI, import SUPPORTED_SERIES_IDS into
extract_series.py and use it in the comprehension (the expression currently
filtering by s.id in ["me","sv","swsh","sm"]), and update extract_sets and CLI
to import the same constant so all code references the single source of truth.
In `@card_data/pipelines/poke_cli_dbt/models/sources.yml`:
- Around line 105-112: Add minimal column contracts for the three new sources by
adding a columns: section under each source definition (standings, tournaments,
country_codes). For standings, declare key columns like tournament_id,
player_id, standing_rank (or rank) and any date field; for tournaments, declare
tournament_id, tournament_name, start_date (and end_date if present); for
country_codes, declare country_code and country_name. Use the "columns:" YAML
list with name and description entries for each column so downstream schema
checks and docs pick them up.
In `@cmd/card/serieslist.go`:
- Around line 63-69: seriesIDMap and the items slice in SeriesList duplicate the
same series definitions; consolidate into one ordered source of truth (e.g., a
single []string or []struct with ID/label) and generate both the seriesIDMap and
the list.Item slice from that single definition. Update the SeriesList()
function to iterate the canonical series slice to build items (using item(...))
and rebuild seriesIDMap from the same loop (or a small helper like
buildSeriesIDMap), and remove the duplicate hard-coded entries so
additions/removals only occur in the one canonical series definition.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
card_data/pipelines/definitions.pycard_data/pipelines/defs/extract/tcgcsv/extract_pricing.pycard_data/pipelines/defs/extract/tcgdex/extract_cards.pycard_data/pipelines/defs/extract/tcgdex/extract_series.pycard_data/pipelines/defs/extract/tcgdex/extract_sets.pycard_data/pipelines/poke_cli_dbt/models/sets.sqlcard_data/pipelines/poke_cli_dbt/models/sources.ymlcard_data/pipelines/soda/checks_series.ymlcard_data/pipelines/soda/checks_sets.ymlcmd/card/serieslist.gocmd/card/serieslist_test.go
✅ Files skipped from review due to trivial changes (1)
- card_data/pipelines/soda/checks_series.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit