1.8.6 #232
1.8.6 #232
Conversation
📝 WalkthroughWalkthroughVersion bumped v1.8.5 → v1.8.6 across release tooling and docs; dbt model/view and CLI extended to propagate a new regulation_mark field; search parsing upgraded to support regex and fuzzy matching; dependency pins and small test/CLI presentation tweaks applied. Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant API
participant DBT as "dbt model / DB"
participant View
CLI->>API: Request card list
API-->>CLI: Returns rows including regulation_mark
Note over DBT,View: dbt materializes regulation_mark into model/table
DBT->>View: Populate/refresh model with regulation_mark
CLI->>View: Query view or render API data including regulation_mark
View-->>CLI: Returns rows with regulation_mark for display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)cmd/search/parse.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting 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 |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @card_data/pyproject.toml:
- Around line 13-14: The dev dependency-group currently allows different
versions (e.g., dagster-postgres>=0.27.3 and dagster-webserver unconstrained)
which can diverge from the pinned production deps; update the dev group to pin
the same exact versions as the main dependencies by changing dagster-postgres to
dagster-postgres==0.28.7 and dagster-webserver to dagster-webserver==1.12.7 in
the dev dependency-group so dev and production installs are consistent.
In @cmd/ability/ability.go:
- Around line 100-105: The code writes the same error twice: remove the
redundant output.WriteString call inside the block that calls
flags.PokemonAbilitiesFlag so only utils.HandleFlagError(&output, err) writes
the error; update the branch that checks *af.Pokemon || *af.ShortPokemon (the
call to flags.PokemonAbilitiesFlag and subsequent error handling) by deleting
the output.WriteString(fmt.Sprintf("error parsing flags: %v\n", err)) line and
keeping the existing return utils.HandleFlagError(&output, err) to avoid
duplicate messages.
In @testdata/main_latest_flag.golden:
- Line 5: Update the golden test output in testdata/main_latest_flag.golden to
match the bumped version: replace the current displayed version string "v1.8.5"
with "v1.8.6" so the CLI --latest flag golden matches other files in the PR and
expected test output.
🧹 Nitpick comments (4)
cmd/search/parse_test.go (1)
19-131: Comprehensive regex test coverage.The subtests thoroughly cover regex scenarios: anchors (
^,$), character classes, alternation, quantifiers (+,?), and error handling for invalid patterns.One consideration: these tests implicitly assert that
parseSearchpreserves the input order of matching results. If this is intentional behavior, consider adding a brief comment documenting this contract. If order isn't guaranteed by the implementation, the assertions checkingfiltered[i].Name != expected[i]could become flaky.cmd/card/cardlist.go (1)
164-170: Minor formatting inconsistency.Line 168 has extra whitespace before
regulationMarkMap. While not a functional issue, consider aligning with the other field assignments for consistency.🔧 Suggested fix
return CardsModel{ IllustratorMap: illustratorMap, ImageMap: imageMap, PriceMap: priceMap, - RegulationMarkMap: regulationMarkMap, + RegulationMarkMap: regulationMarkMap, Table: t, }, nilcmd/search/parse.go (2)
25-27: Regex detection may cause unexpected behavior for common search terms.The heuristic triggers regex mode for any query containing characters like
.,*,+,?, etc. A user searching for"mr. mime"or"farfetch'd"(if apostrophe were listed) would unintentionally trigger regex mode. The period (.) is especially common in names but also a regex wildcard.Consider either:
- Requiring an explicit prefix/flag for regex mode (e.g.,
r/pattern)- Escaping special characters when not in regex mode
- Documenting this behavior clearly for users
60-66: Slice reuse mutates the original backing array.
filtered := results[:0]creates a slice that shares the same backing array asresults. Appending tofilteredoverwrites elements in the original array. While this works in the current call site becauseresult.Resultsis reassigned, it's error-prone if the code is refactored or ifresultsis used elsewhere concurrently.🔧 Safer approach: allocate a new slice
- filtered := results[:0] + filtered := make([]Result, 0, len(matches)) for _, r := range results { if _, ok := matchSet[r.Name]; ok { filtered = append(filtered, r) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
card_data/data_infrastructure_v2.pngis excluded by!**/*.pngcard_data/uv.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.github/workflows/ci.yml.goreleaser.ymlDockerfileREADME.mdcard_data/README.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pipelines/poke_cli_dbt/macros/create_view.sqlcard_data/pipelines/poke_cli_dbt/models/cards.sqlcard_data/pyproject.tomlcli.gocmd/ability/ability.gocmd/card/cardlist.gocmd/card/cardlist_test.gocmd/card/setslist_test.gocmd/search/parse.gocmd/search/parse_test.gogo.modnfpm.yamltestdata/cli_help.goldentestdata/cli_incorrect_command.goldentestdata/main_latest_flag.golden
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/ability/ability.go (1)
cmd/utils/output.go (1)
HandleFlagError(24-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: benchmarks
🔇 Additional comments (28)
cli.go (3)
27-49: Clean refactor for command list rendering.The introduction of
commandDescriptionsandrenderCommandList()effectively centralizes command metadata and eliminates duplication. Usingstrings.Builderis the idiomatic and efficient approach for string concatenation in Go.
96-97: LGTM!The integration of
renderCommandList()into the help message maintains a single source of truth for command descriptions.
164-177: LGTM!The error handling for invalid commands now uses the same
renderCommandList()function, ensuring users see consistent command information whether viewing help or encountering errors.testdata/cli_help.golden (1)
1-32: Golden file correctly reflects the refactored help output.The command list and formatting align with the
commandDescriptionsdata structure andrenderCommandList()function incli.go.testdata/cli_incorrect_command.golden (1)
1-18: Golden file correctly reflects the refactored error output.The error message format and command list are consistent with both
cli.goandcli_help.golden, demonstrating the benefit of therenderCommandList()refactor for maintaining consistency.Dockerfile (1)
11-11: LGTM!Version bump to v1.8.6 is consistent with the PR objective. The Dockerfile maintains good security practices with multi-stage builds and non-root user execution.
card_data/pipelines/poke_cli_dbt/dbt_project.yml (2)
2-2: LGTM!Version bump to 1.8.6 aligns with the broader release.
18-24: Verify the new materialization default.The
+materialized: tableconfiguration will materialize all models underpoke_cli_dbtas tables by default. Confirm this is intentional and won't cause issues with any models that should remain views or incremental.Note: The AI summary indicated the
on-run-endhook was removed, but it's still present (lines 23-24)..goreleaser.yml (1)
17-17: LGTM!Version bump to v1.8.6 is consistent with Dockerfile and other build configurations.
card_data/README.md (1)
10-10: LGTM!The switch to a local image reference is appropriate for documentation maintainability. The image file
data_infrastructure_v2.pngexists in thecard_data/directory as expected.README.md (1)
5-5: LGTM!Version references updated consistently to v1.8.6 across the Docker image badge and usage examples.
Also applies to: 99-99, 103-103
.github/workflows/ci.yml (1)
34-34: LGTM!The
VERSION_NUMBERenvironment variable is the single source of truth for the version across the CI workflow. The bump tov1.8.6is correctly propagated to all build, packaging, and deployment steps.nfpm.yaml (1)
4-4: LGTM!Version bump to
v1.8.6in packaging metadata aligns with the coordinated release across the project.go.mod (1)
50-50: Theclosestmatchdependency is actively used and necessary.This library is used in
cmd/search/parse.gofor fuzzy string matching with specific bag size parameters (2, 3, 4). While the+incompatiblesuffix indicates it's a pre-Go-modules package and hasn't been updated in several years, it's functional for the use case and actively utilized in the codebase.Note:
sahilm/fuzzyexists in dependencies but is not used anywhere in the codebase, so it cannot serve as a replacement forclosestmatch.card_data/pipelines/poke_cli_dbt/models/cards.sql (1)
6-8: Verify that filtering non-numericlocalIdvalues is intentional.The WHERE clause
"localId" ~ '^[0-9]+$'will exclude cards with alphanumeric identifiers (e.g., promo cards like "SWSH001", "TG01", or special edition cards). Ensure this doesn't inadvertently filter out legitimate card data that should be included in the pipeline.The
regulation_markcolumn addition looks good.#!/bin/bash # Check if there are cards with non-numeric localId values that would be filtered out # This helps understand the impact of the WHERE clause # Look for related SQL files or documentation about localId format fd -e sql -e md | xargs rg -l "localId" 2>/dev/null | head -10 # Check for any comments or documentation about the localId filtering rg -n "localId" --type sql -C2cmd/card/setslist_test.go (1)
195-198: LGTM!The use of the shared
testSupabaseKeyconstant fromcardlist_test.gois appropriate since both test files belong to the samecardpackage. This improves maintainability by centralizing the test API key.card_data/pipelines/poke_cli_dbt/macros/create_view.sql (1)
13-14: LGTM!The
regulation_markfield is correctly propagated through the CTE and final SELECT. The addition ofset_nameto the CTE aligns with its existing usage in the outer SELECT at line 30.Also applies to: 34-35
cmd/card/cardlist_test.go (2)
14-17: LGTM!Good refactoring to extract the test API key into a named constant. The documentation comment clearly explains the purpose, and centralizing this value improves maintainability across the test suite.
312-315: LGTM!Header assertions correctly reference the centralized constant.
cmd/search/parse_test.go (2)
11-16: LGTM!Good expansion of the mock data to provide sufficient coverage for the various regex pattern tests.
150-151: LGTM!The query test correctly validates regex filtering with the
.*chu.*pattern matching both "pikachu" and "raichu".cmd/card/cardlist.go (4)
16-27: LGTM!The
CardsModelstruct extension withRegulationMarkMapfollows the same pattern as existing maps (IllustratorMap,ImageMap,PriceMap) for consistency.
74-76: LGTM!The view rendering properly retrieves the regulation mark from the map and displays it alongside other card details. The fallback value is set during map population in
CardsList, maintaining consistency with theillustratorfield handling pattern.
95-102: LGTM!The
cardDatastruct and API query are correctly extended to includeregulation_mark. The JSON tag matches the expected API response field name.Also applies to: 106-106
138-143: LGTM!The regulation mark handling mirrors the pattern used for
illustrator, providing a clear fallback message when the field is empty.cmd/search/parse.go (3)
29-42: LGTM with a note on regex safety.The implementation is correct. Go's
regexppackage uses a linear-time matching algorithm (RE2-based), so it's inherently safe from ReDoS attacks even with user-supplied patterns.
69-75: LGTM!The refactored
parseSearchcleanly delegates to the appropriate strategy based on query content and properly propagates errors from the regex path.
86-89: LGTM!Error handling is properly integrated into the
queryfunction, ensuring regex compilation failures are surfaced to callers.
Summary by CodeRabbit
New Features
Bug Fixes / Data
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.