Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR bumps project version from v1.9.3 to v1.9.4 across CI, build, packaging, and docs; adds exported version-group constants; refactors Pokémon rendering into a new render module with tests; and adds a User-Agent header to TCGCSV HTTP requests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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 (8)
flags/pokemonflagset.go (1)
22-22: LGTM.
constants.VersionScarletVioletreplacement keeps filter semantics unchanged. Optional follow-up: consider also extracting"level-up"as a constant (e.g.,MoveLearnedMethodLevelUp) for symmetry if more learn-method filtering is added later.Also applies to: 431-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/pokemonflagset.go` at line 22, The code currently uses the string literal "level-up" alongside constants.VersionScarletViolet; extract that literal into a new constant (e.g., MoveLearnedMethodLevelUp) in the constants package and replace all uses of the "level-up" literal (including where learn-method filtering occurs) with constants.MoveLearnedMethodLevelUp so learn-method filtering remains symmetric with version constants and easier to extend later.constants/values.go (1)
3-8: Remove unused constantsVersionSunMoonandVersionXY.These constants have no usages in the codebase. Either remove them per YAGNI, or add the intended call sites in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@constants/values.go` around lines 3 - 8, Remove the unused constants VersionSunMoon and VersionXY from the const block that defines VersionScarletViolet and VersionSwordShield (i.e., delete the VersionSunMoon and VersionXY entries in the values.go const list) and run a build/grep to confirm there are no remaining references; if those constants were intended to be used, instead add the missing call sites that reference VersionSunMoon and/or VersionXY so they are actually exercised..github/workflows/ci.yml (1)
34-34: Consider centralizing version source to reduce drift risk.Line 34 is correct, but version strings are still duplicated across multiple files. A single source (e.g., tag-driven or generated version file) would make future bumps safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 34, The VERSION_NUMBER literal is duplicated and should be centralized: create a single authoritative source (e.g., a VERSION file, package metadata, or derive from git tag) and update the CI workflow (where VERSION_NUMBER is used) to read that source (env var or file) instead of hardcoding; then change all other places that currently duplicate the version string to reference the same source so bumps happen once and consistently; ensure the workflow sets an environment variable (e.g., VERSION) from the chosen source and that any scripts/builds consume that variable.structs/structs_test.go (1)
10-17: Good test coverage for GetResourceName() methods.The test correctly validates all six struct types'
GetResourceName()implementations with realistic test data.Optional suggestion: Consider table-driven test pattern.
Go convention favors table-driven tests for better maintainability. This would also make it easier to add edge cases (e.g., empty strings, special characters).
♻️ Optional refactor to table-driven test
func TestGetResourceName(t *testing.T) { - assert.Equal(t, "strong-jaw", AbilityJSONStruct{Name: "strong-jaw"}.GetResourceName()) - assert.Equal(t, "poke-ball", ItemJSONStruct{Name: "poke-ball"}.GetResourceName()) - assert.Equal(t, "thunderbolt", MoveJSONStruct{Name: "thunderbolt"}.GetResourceName()) - assert.Equal(t, "pikachu", PokemonJSONStruct{Name: "pikachu"}.GetResourceName()) - assert.Equal(t, "pikachu", PokemonSpeciesJSONStruct{Name: "pikachu"}.GetResourceName()) - assert.Equal(t, "fire", TypesJSONStruct{Name: "fire"}.GetResourceName()) + tests := []struct { + name string + resource interface{ GetResourceName() string } + expected string + }{ + {"Ability", AbilityJSONStruct{Name: "strong-jaw"}, "strong-jaw"}, + {"Item", ItemJSONStruct{Name: "poke-ball"}, "poke-ball"}, + {"Move", MoveJSONStruct{Name: "thunderbolt"}, "thunderbolt"}, + {"Pokemon", PokemonJSONStruct{Name: "pikachu"}, "pikachu"}, + {"PokemonSpecies", PokemonSpeciesJSONStruct{Name: "pikachu"}, "pikachu"}, + {"Type", TypesJSONStruct{Name: "fire"}, "fire"}, + {"EmptyName", AbilityJSONStruct{Name: ""}, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.resource.GetResourceName()) + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@structs/structs_test.go` around lines 10 - 17, Refactor TestGetResourceName into a table-driven test: create a slice of test cases containing fields for name, struct type (AbilityJSONStruct, ItemJSONStruct, MoveJSONStruct, PokemonJSONStruct, PokemonSpeciesJSONStruct, TypesJSONStruct), and expected resource string, then iterate over them with t.Run to call each struct's GetResourceName() and assert equality; this centralizes cases, makes adding edge cases (empty string, special characters) easier, and keeps test logic in the same TestGetResourceName function.cmd/pokemon/render.go (4)
21-50: Move the lookup maps to package-level vars to avoid re-allocating on every call.
modernEggInformationNames(Lines 21-27) andm(Lines 39-50) are rebuilt every timerenderEggInformationruns. Same pattern appears fornameMappinginrenderEffortValues(Lines 64-71). Since the command is short-lived this has no real perf impact, but hoisting them tovar (...)at package scope improves readability and signals that they are constants. Also,mis a non-descriptive name — considergenderRateLabelsor similar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/render.go` around lines 21 - 50, The lookup maps modernEggInformationNames, m, and nameMapping are being allocated inside renderEggInformation and renderEffortValues on every call; hoist them to package-level vars (var (...) block) so they are allocated once, give the map currently named m a descriptive identifier like genderRateLabels, and update renderEggInformation and renderEffortValues to reference these package-level symbols instead of re-creating them locally.
88-97:renderEntrysilently renders nothing when no preferred-version English entry exists.If the species has flavor text only for versions outside
{x, shield, scarlet}(e.g. older or newer games, or a Pokémon that was never in those titles), this function writes nothing and the overall output just shows the name with no description. Consider a fallback to the first English entry when no preferred version matches, so the user always sees some Pokédex text:♻️ Suggested fallback
func renderEntry(w io.Writer, s structs.PokemonSpeciesJSONStruct) { - for _, entry := range s.FlavorTextEntries { - if entry.Language.Name == "en" && (entry.Version.Name == "x" || entry.Version.Name == "shield" || entry.Version.Name == "scarlet") { - flavorText := strings.ReplaceAll(entry.FlavorText, "\n", " ") - flavorText = strings.Join(strings.Fields(flavorText), " ") - fmt.Fprintln(w, utils.WrapText(flavorText, 60)) - return - } - } + var fallback string + for _, entry := range s.FlavorTextEntries { + if entry.Language.Name != "en" { + continue + } + if entry.Version.Name == "x" || entry.Version.Name == "shield" || entry.Version.Name == "scarlet" { + flavorText := strings.Join(strings.Fields(strings.ReplaceAll(entry.FlavorText, "\n", " ")), " ") + fmt.Fprintln(w, utils.WrapText(flavorText, 60)) + return + } + if fallback == "" { + fallback = strings.Join(strings.Fields(strings.ReplaceAll(entry.FlavorText, "\n", " ")), " ") + } + } + if fallback != "" { + fmt.Fprintln(w, utils.WrapText(fallback, 60)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/render.go` around lines 88 - 97, renderEntry currently returns without writing anything if no English flavor text matches the preferred versions, causing missing descriptions; modify renderEntry to first look for an English entry with Version.Name in {"x","shield","scarlet"} and print it as now, but if none found, fall back to the first entry where Language.Name == "en" (e.g. iterate FlavorTextEntries again or store the first English candidate) before returning; keep the existing newline/whitespace normalization and call to utils.WrapText(flavorText, 60) and ensure the function still returns after printing.
63-86:renderEffortValuesalways emits the label even when there are no EVs.When a Pokémon has no positive-effort stats (e.g. early-game or unusual entries),
evsstays nil and the output becomes"\n• Effort Values: "with a trailing space and nothing after it. The existing test"no EVs produces empty list"actually locks this in. That's a minor UX glitch — consider either omitting the line entirely or printing a placeholder likeNone:🎨 Optional polish
- fmt.Fprintf(w, "\n%s Effort Values: %s", styling.ColoredBullet, strings.Join(evs, ", ")) + joined := "None" + if len(evs) > 0 { + joined = strings.Join(evs, ", ") + } + fmt.Fprintf(w, "\n%s Effort Values: %s", styling.ColoredBullet, joined)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/render.go` around lines 63 - 86, renderEffortValues currently always prints the "Effort Values" label even when the evs slice is empty; update the function (renderEffortValues) to check if evs is empty after building it and either skip the fmt.Fprintf entirely when len(evs) == 0 or print a placeholder such as "None" instead of an empty list so the output does not show a trailing empty label; ensure you reference the evs slice (and keep nameMapping/stat handling unchanged) so tests expecting a different behavior can be adjusted if needed.
128-148:renderTypingalways emits a trailing newline even when no types render.If none of
s.Typesmatchstyling.ColorMap(ors.Typesis empty),typeBoxesis nil andlipgloss.JoinHorizontalreturns"", butfmt.Fprintlnat Line 147 still writes a bare"\n". Guard the call to keep the composed output fromcmd/pokemon/pokemon.go(Lines 97-100) clean:♻️ Suggested guard
- fmt.Fprintln(w, lipgloss.JoinHorizontal(lipgloss.Top, typeBoxes...)) + if len(typeBoxes) > 0 { + fmt.Fprintln(w, lipgloss.JoinHorizontal(lipgloss.Top, typeBoxes...)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pokemon/render.go` around lines 128 - 148, renderTyping currently always writes a newline because it unconditionally calls fmt.Fprintln with lipgloss.JoinHorizontal even when typeBoxes is empty; change renderTyping to compute the joined string (e.g., joined := lipgloss.JoinHorizontal(lipgloss.Top, typeBoxes...)) and only call fmt.Fprintln(w, joined) when joined != "" (or when len(typeBoxes) > 0) so no trailing newline is emitted when there are no rendered types; update references in renderTyping (typeBoxes, pokeType, colorHex, typeColorStyle) accordingly.
🤖 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/defs/extract/tcgcsv/extract_pricing.py`:
- Line 140: Replace the hardcoded "poke-cli/1.9" header by extracting a
module-level USER_AGENT constant that includes the full package version
(including patch) from the installed package metadata (e.g., via
importlib.metadata.version("card_data") or equivalent); define USER_AGENT =
f"card_data/{version}" at top of the module and then set headers =
{"User-Agent": USER_AGENT} in the request logic (refer to the headers variable
and the extract_pricing.py module).
In `@cmd/pokemon/render_test.go`:
- Around line 313-340: The test's assertion for the "unknown type is skipped"
case is too weak; update the test in cmd/pokemon/render_test.go so that when
tt.contains == "" (the unknown-type case) it asserts the output is empty after
trimming whitespace (i.e., assert.Empty(t, strings.TrimSpace(output))) instead
of the current disjunctive check; locate the assertion around the renderTyping
test loop (function renderTyping and the test block using buf, output, and
tt.contains) and replace the weak check with a direct empty-string assertion to
ensure unknown types produce no rendered output.
- Around line 18-49: The test "scarlet entry preferred" in render_test.go
contradicts its assertion: it currently validates that renderEntry returns the
first matching English entry ("A shield entry.") but the name implies scarlet
should be preferred; either rename the test to reflect current behavior (e.g.,
"first matching english entry wins") and add a case where scarlet appears first
to lock ordering, or change the selection logic in renderEntry (in
cmd/pokemon/render.go) to use a version-priority list (e.g.,
["scarlet","shield",...]) and, for each preferred version in order, scan
FlavorTextEntries for language=="en" and version.Name==that preferred version
and return the match; update tests accordingly.
In `@testdata/main_latest_flag.golden`:
- Line 5: Update the golden fixture by replacing the pinned version string
"v1.9.3" with "v1.9.4" in main_latest_flag.golden so the snapshot matches the
repo-wide version bump referenced in the release/CI files; locate the line
containing "v1.9.3" and change it to "v1.9.4".
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 34: The VERSION_NUMBER literal is duplicated and should be centralized:
create a single authoritative source (e.g., a VERSION file, package metadata, or
derive from git tag) and update the CI workflow (where VERSION_NUMBER is used)
to read that source (env var or file) instead of hardcoding; then change all
other places that currently duplicate the version string to reference the same
source so bumps happen once and consistently; ensure the workflow sets an
environment variable (e.g., VERSION) from the chosen source and that any
scripts/builds consume that variable.
In `@cmd/pokemon/render.go`:
- Around line 21-50: The lookup maps modernEggInformationNames, m, and
nameMapping are being allocated inside renderEggInformation and
renderEffortValues on every call; hoist them to package-level vars (var (...)
block) so they are allocated once, give the map currently named m a descriptive
identifier like genderRateLabels, and update renderEggInformation and
renderEffortValues to reference these package-level symbols instead of
re-creating them locally.
- Around line 88-97: renderEntry currently returns without writing anything if
no English flavor text matches the preferred versions, causing missing
descriptions; modify renderEntry to first look for an English entry with
Version.Name in {"x","shield","scarlet"} and print it as now, but if none found,
fall back to the first entry where Language.Name == "en" (e.g. iterate
FlavorTextEntries again or store the first English candidate) before returning;
keep the existing newline/whitespace normalization and call to
utils.WrapText(flavorText, 60) and ensure the function still returns after
printing.
- Around line 63-86: renderEffortValues currently always prints the "Effort
Values" label even when the evs slice is empty; update the function
(renderEffortValues) to check if evs is empty after building it and either skip
the fmt.Fprintf entirely when len(evs) == 0 or print a placeholder such as
"None" instead of an empty list so the output does not show a trailing empty
label; ensure you reference the evs slice (and keep nameMapping/stat handling
unchanged) so tests expecting a different behavior can be adjusted if needed.
- Around line 128-148: renderTyping currently always writes a newline because it
unconditionally calls fmt.Fprintln with lipgloss.JoinHorizontal even when
typeBoxes is empty; change renderTyping to compute the joined string (e.g.,
joined := lipgloss.JoinHorizontal(lipgloss.Top, typeBoxes...)) and only call
fmt.Fprintln(w, joined) when joined != "" (or when len(typeBoxes) > 0) so no
trailing newline is emitted when there are no rendered types; update references
in renderTyping (typeBoxes, pokeType, colorHex, typeColorStyle) accordingly.
In `@constants/values.go`:
- Around line 3-8: Remove the unused constants VersionSunMoon and VersionXY from
the const block that defines VersionScarletViolet and VersionSwordShield (i.e.,
delete the VersionSunMoon and VersionXY entries in the values.go const list) and
run a build/grep to confirm there are no remaining references; if those
constants were intended to be used, instead add the missing call sites that
reference VersionSunMoon and/or VersionXY so they are actually exercised.
In `@flags/pokemonflagset.go`:
- Line 22: The code currently uses the string literal "level-up" alongside
constants.VersionScarletViolet; extract that literal into a new constant (e.g.,
MoveLearnedMethodLevelUp) in the constants package and replace all uses of the
"level-up" literal (including where learn-method filtering occurs) with
constants.MoveLearnedMethodLevelUp so learn-method filtering remains symmetric
with version constants and easier to extend later.
In `@structs/structs_test.go`:
- Around line 10-17: Refactor TestGetResourceName into a table-driven test:
create a slice of test cases containing fields for name, struct type
(AbilityJSONStruct, ItemJSONStruct, MoveJSONStruct, PokemonJSONStruct,
PokemonSpeciesJSONStruct, TypesJSONStruct), and expected resource string, then
iterate over them with t.Run to call each struct's GetResourceName() and assert
equality; this centralizes cases, makes adding edge cases (empty string, special
characters) easier, and keeps test logic in the same TestGetResourceName
function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e80ec9e-df07-4a4f-9528-729c7f1e5ced
📒 Files selected for processing (17)
.github/workflows/ci.yml.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/defs/extract/tcgcsv/extract_pricing.pycard_data/pipelines/poke_cli_dbt/dbt_project.ymlcmd/item/item.gocmd/move/move.gocmd/pokemon/pokemon.gocmd/pokemon/render.gocmd/pokemon/render_test.goconstants/values.goflags/pokemonflagset.gonfpm.yamlstructs/structs_test.gotestdata/main_latest_flag.goldenweb/pyproject.toml
This was
linked to
issues
Apr 23, 2026
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
Chores
Bug Fixes
Improvements
Tests