1.8.0 #203
1.8.0 #203
Conversation
WalkthroughAdds a new interactive "card" CLI (series → sets → cards → image viewer with Sixel), bumps project/version metadata to v1.8.0 across CI/build files, updates dbt models/macros and pipelines (adds Soda data-quality checks), extends pricing extraction mappings, refreshes dependencies, and updates docs and ignore rules. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CardCommand
participant Series as SeriesList
participant Sets as SetsList
participant Cards as CardsList
participant Viewer as ImageViewer
User->>CLI: run `poke-cli card`
CLI->>Series: launch SeriesList
Series->>User: show series list
User->>Series: select series
Series-->>CLI: return SeriesID
CLI->>Sets: launch SetsList(SeriesID)
Sets->>Sets: fetch sets (HTTP)
Sets->>User: show sets
User->>Sets: select set
Sets-->>CLI: return SetID
CLI->>Cards: launch CardsList(SetID)
Cards->>Cards: fetch cards & prices (HTTP)
Cards->>User: show table + details
User->>Cards: press space to view image
Cards->>Viewer: launch ImageViewer (CardImage)
Viewer->>Viewer: fetch/resize/encode Sixel
Viewer->>User: display image
User->>Viewer: quit (Esc/Ctrl+C)
Viewer-->>Cards: return to cards view
User->>Cards: quit -> CLI returns output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cmd/card/card.go (6)
⏰ 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 (2)
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
🚀 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: 11
🧹 Nitpick comments (9)
card_data/pyproject.toml (1)
45-49: Add documentation explaining why these uv overrides are necessary.The overrides are legitimate—both
deepdiffandstarlettedo appear as transitive dependencies inuv.lock. However, without documented justification for pinning these specific versions, this becomes a maintainability concern for future developers.Consider adding an explanatory comment:
[tool.uv] +# Override transitive dependencies from dagster/dbt ecosystem to maintain compatibility override-dependencies = [ "deepdiff==8.6.1", "starlette==0.49.1", ]This preserves context about why these pins exist and prevents accidental removal.
card_data/pipelines/defs/extract/extract_data.py (1)
92-107: Switch tome02-only card URL extraction: confirm intended scope
extract_card_url_from_setnow hard-codes only theme02set inurls, so downstream assets will exclusively process cards from this set. If the goal was to addme02coverage rather than replace the previously targeted set(s), you’ll want to include the prior URLs alongsideme02in the list so existing coverage isn’t silently dropped.card_data/pipelines/defs/extract/extract_pricing_data.py (1)
75-79: ExcludingReverse Holofoilfromprice_dictchanges pricing semanticsFiltering out entries where
subTypeName == "Reverse Holofoil"means your pricing DataFrame will now represent non-reverse variants only. That’s likely desired, but it will:
- Drop Reverse Holofoil rows entirely where they were previously included.
- Change aggregate stats or counts if downstream logic assumed all printings were present.
Confirm this aligns with reporting/analytics expectations, and consider extracting
"Reverse Holofoil"to a constant if you plan to tune subtype handling further.README.md (1)
219-219: Add blank line before table.Markdown best practices recommend surrounding tables with blank lines for better readability and parser compatibility.
Based on static analysis hints, add a blank line before the table:
--- ## Tested Terminals + | Terminal | OS | Status | Issues |cmd/card/cardinfo.go (1)
23-58: Solid implementation with proper security checks.
CardImagecorrectly validates URLs, enforces timeouts and size limits, and handles errors appropriately. The 10 MB limit and 15-second timeout provide reasonable protection against abuse.Consider adding context support for better cancellation control:
func CardImage(ctx context.Context, imageURL string) (string, error) { client := &http.Client{ Timeout: time.Second * 15, } req, err := http.NewRequestWithContext(ctx, "GET", imageURL, nil) if err != nil { return "", err } parsedURL, err := url.Parse(imageURL) if err != nil || (parsedURL.Scheme != "http" && parsedURL.Scheme != "https") { return "", errors.New("invalid URL scheme") } resp, err := client.Do(req) // ... rest of implementation }This would allow callers to cancel long-running requests if needed.
cmd/card/design.go (1)
31-35: Silent type assertion could hide bugs.The type assertion fails silently with an empty return. While this is a common pattern in Bubble Tea delegates for graceful degradation, it could mask bugs during development if an incorrect type is passed to the Render method.
Consider adding a debug log or assertion during development to catch type mismatches early.
cmd/card/design_test.go (1)
80-87: Remove redundant check.The checks at lines 81 and 85 are equivalent—
output == ""andlen(output) == 0both test for an empty string. Keep only one.output := buf.String() if output == "" { t.Error("Expected non-empty output for selected item") } - - if len(output) == 0 { - t.Error("Selected item should produce rendered output") - }cmd/card/card.go (2)
17-35: Consider using a dedicated FlagSet for subcommand isolation.Using
flag.Parse()operates on global state, which can cause issues in multi-command CLIs where each subcommand might define its own flags. Creating a dedicatedflag.NewFlagSet("card", flag.ContinueOnError)would isolate this command's flags.+func CardCommand() (string, error) { + var output strings.Builder + flagSet := flag.NewFlagSet("card", flag.ContinueOnError) + + flagSet.Usage = func() { helpMessage := styling.HelpBorder.Render( // ... ) output.WriteString(helpMessage) } - - flag.Parse() - - // Handle help flag - if len(os.Args) == 3 && (os.Args[2] == "-h" || os.Args[2] == "--help") { - flag.Usage() + + if err := flagSet.Parse(os.Args[2:]); err != nil { + flagSet.Usage() return output.String(), nil }
83-94: Consider validating imageURL before launching viewer.If
cardsResult.SelectedOptionis not found inImageMap, an empty string will be passed toImageRenderer. Consider adding validation to provide a better user experience.if cardsResult.ViewImage { // Launch image viewer imageURL := cardsResult.ImageMap[cardsResult.SelectedOption] + if imageURL == "" { + fmt.Fprintf(os.Stderr, "Warning: no image available for %s\n", cardsResult.SelectedOption) + cardsResult.ViewImage = false + cardsModel = cardsResult + continue + } imageModel := ImageRenderer(cardsResult.SelectedOption, imageURL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
card_data/uv.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks).gitleaksignore(1 hunks).goreleaser.yml(1 hunks)Dockerfile(1 hunks)README.md(6 hunks)card_data/dagster.yaml(1 hunks)card_data/pipelines/defs/extract/extract_data.py(2 hunks)card_data/pipelines/defs/extract/extract_pricing_data.py(2 hunks)card_data/pipelines/poke_cli_dbt/dbt_project.yml(1 hunks)card_data/pipelines/poke_cli_dbt/macros/create_rls.sql(1 hunks)card_data/pipelines/poke_cli_dbt/macros/create_view.sql(1 hunks)card_data/pipelines/poke_cli_dbt/models/cards.sql(1 hunks)card_data/pipelines/poke_cli_dbt/models/pricing_data.sql(1 hunks)card_data/pyproject.toml(1 hunks)cli.go(4 hunks)cmd/berry/berryinfo.go(0 hunks)cmd/card/card.go(1 hunks)cmd/card/card_test.go(1 hunks)cmd/card/cardinfo.go(1 hunks)cmd/card/cardinfo_test.go(1 hunks)cmd/card/cardlist.go(1 hunks)cmd/card/cardlist_test.go(1 hunks)cmd/card/design.go(1 hunks)cmd/card/design_test.go(1 hunks)cmd/card/imageviewer.go(1 hunks)cmd/card/imageviewer_test.go(1 hunks)cmd/card/serieslist.go(1 hunks)cmd/card/serieslist_test.go(1 hunks)cmd/card/setslist.go(1 hunks)cmd/card/setslist_test.go(1 hunks)cmd/search/model_input.go(1 hunks)cmd/search/model_input_test.go(1 hunks)cmd/search/model_selection.go(1 hunks)cmd/search/parse.go(1 hunks)cmd/search/search.go(2 hunks)cmd/search/search_test.go(1 hunks)cmd/utils/validateargs.go(2 hunks)cmd/utils/validateargs_test.go(2 hunks)go.mod(1 hunks)nfpm.yaml(1 hunks)testdata/cli_help.golden(1 hunks)testdata/cli_incorrect_command.golden(1 hunks)testdata/main_latest_flag.golden(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/berry/berryinfo.go
🧰 Additional context used
🧬 Code graph analysis (9)
cli.go (2)
cmd/utils/output.go (1)
HandleCommandOutput(12-22)cmd/card/card.go (1)
CardCommand(14-103)
cmd/card/card_test.go (1)
cmd/card/card.go (1)
CardCommand(14-103)
cmd/card/imageviewer.go (1)
cmd/card/cardinfo.go (1)
CardImage(23-58)
cmd/card/cardlist_test.go (1)
cmd/card/cardlist.go (1)
CardsModel(15-25)
cmd/card/card.go (6)
styling/styling.go (2)
HelpBorder(31-33)StyleBold(28-28)cmd/utils/validateargs.go (1)
ValidateCardArgs(59-69)cmd/card/serieslist.go (2)
SeriesList(62-81)SeriesModel(14-19)cmd/card/setslist.go (2)
SetsList(74-108)SetsModel(14-21)cmd/card/cardlist.go (2)
CardsList(101-149)CardsModel(15-25)cmd/card/imageviewer.go (1)
ImageRenderer(31-38)
cmd/card/serieslist_test.go (1)
cmd/card/serieslist.go (1)
SeriesModel(14-19)
cmd/card/cardlist.go (1)
styling/styling.go (3)
TypesTableBorder(42-44)Color(84-86)KeyMenu(22-22)
cmd/card/cardinfo_test.go (1)
cmd/card/cardinfo.go (1)
CardImage(23-58)
cmd/utils/validateargs_test.go (2)
cmd/utils/validateargs.go (1)
ValidateCardArgs(59-69)styling/styling.go (1)
StripANSI(75-78)
🪛 Gitleaks (8.29.0)
cmd/card/cardlist.go
[high] 157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[uncategorized] ~226-~226: The name of this game or TV show is spelled with an accent.
Context: ... | - Some colors aren't supported.
- pokemon <name> --image=xx flag pixel issues. ...
(POKEMON)
[uncategorized] ~227-~227: The name of this game or TV show is spelled with an accent.
Context: ... macOS | 🟠 | - pokemon <name> --image=xx flag pixel issues.<b...
(POKEMON)
🪛 markdownlint-cli2 (0.18.1)
README.md
32-32: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
193-193: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
195-195: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
196-196: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
197-197: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
219-219: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ 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: tests
🔇 Additional comments (40)
card_data/dagster.yaml (2)
14-15: Verify telemetry disabling is intentional and documented.The telemetry configuration is now globally disabled. Ensure that:
- This aligns with your observability strategy and doesn't inadvertently hide operational issues
- If telemetry should differ across environments (dev, staging, production), this should be environment-specific rather than hardcoded
Consider whether this decision is documented in release notes or operational runbooks.
12-13: YAML formatting looks good.The trailing newline and overall YAML structure are correct. The existing PostgreSQL configuration is preserved properly.
.gitignore (1)
63-63: Summary inconsistency: dbt_packages removal claim unverified.The AI summary states "Removed the dbt_packages/ entry from the dbt-related ignore block," but
dbt_packages/is still present at line 49. Please clarify whether this is an outdated summary or if additional changes were intended.The new
/card_data/sample_scripts/entry itself is appropriate and follows the pattern of line 62 (/card_data/supabase/), aligning with the PR's card data pipeline work and preventing sample scripts (potentially containing credentials or test data) from being committed.card_data/pipelines/defs/extract/extract_pricing_data.py (2)
13-28: ExpandedSET_PRODUCT_MATCHING: validate set codes ↔ product IDsThe new entries (sv03+ variants, sv04+ variants, and me01/me02) look consistent with the existing pattern, but their correctness fully depends on external TCGCSV product IDs. A wrong ID will quietly produce incorrect/empty pricing for that set, so it’s worth double-checking these mappings against the upstream source (or adding a quick assertion/health check that each
product_idreturns non-empty results).
68-73: URL construction refactor to f-strings looks goodSwitching
products_urlandprices_urlto f-strings withproduct_idkeeps the same behavior and improves readability. No functional concerns here.card_data/pipelines/poke_cli_dbt/models/cards.sql (1)
6-6: Column added but not utilized in dependent view.The
illustratorcolumn is added to the cards model but is not selected in thecreate_viewmacro'scards_cte. Verify if this is intentional (late-binding by downstream consumers) or if the view should include this field.card_data/pipelines/poke_cli_dbt/models/pricing_data.sql (1)
3-6: Verify thecreate_view()macro executes successfully.The post-hook now chains two operations:
enable_rls()thencreate_view(). Ensure the new macro runs without errors and that the join logic in the view produces expected results (see separate review comment on create_view.sql for a potential correctness issue).card_data/pipelines/poke_cli_dbt/macros/create_rls.sql (1)
3-8: Clear and explicit RLS policy definition.The policy syntax is now more readable with explicit clauses (
AS PERMISSIVE,FOR SELECT,TO PUBLIC). This improves maintainability without changing functional behavior.card_data/pipelines/poke_cli_dbt/dbt_project.yml (1)
23-24: Verify removal ofcreate_relationships()hook.The
on-run-endhook callingcreate_relationships()has been removed. Confirm that:
- This macro is no longer needed or is being replaced by another mechanism
- No relationship tests are orphaned or broken
- This aligns with the shift to model-level post-hooks (e.g., in
pricing_data.sql)card_data/pipelines/poke_cli_dbt/macros/create_view.sql (1)
1-40: Verify hard-coded schema references match dbt materialization targets.The macro references
public.cards(line 13) andpublic.pricing_data(line 22) with hard-coded schema names. Confirm these models are materialized to thepublicschema (typically the default), or update schema references to use dbt's built-in reference mechanism (e.g.,{{ ref('cards') }}).cmd/search/model_selection.go (1)
5-5: LGTM!The added blank line improves import block readability and maintains consistency with standard Go formatting conventions.
cmd/search/parse.go (1)
5-6: LGTM!The import reordering improves consistency by grouping third-party imports separately from standard library imports.
cmd/search/model_input_test.go (1)
4-4: LGTM!The missing
testingimport has been correctly added, which is required for thetesting.Tparameter inTestUpdateInput.testdata/cli_incorrect_command.golden (1)
8-8: LGTM!The addition of the "card" command to the help output correctly reflects the new CLI feature introduced in this PR.
cmd/search/model_input.go (1)
5-6: LGTM!The import reordering improves consistency by grouping standard library imports together before third-party imports.
cmd/search/search_test.go (1)
10-12: LGTM!The import reordering improves consistency by grouping third-party imports appropriately.
testdata/main_latest_flag.golden (1)
5-5: Disregard this review comment - the golden file is correct.The test queries the actual GitHub API to fetch the latest release (
https://api.github.com/repos/digitalghost-dev/poke-cli/releases/latest), which currently returns v1.7.4. The golden file correctly reflects this real API response. The PR bumps the internal application version to v1.8.0, but this doesn't make v1.8.0 the latest release on GitHub until a git tag for v1.8.0 is created and the release workflow publishes it. The golden file should remain as v1.7.4.Likely an incorrect or invalid review comment.
cmd/search/search.go (1)
76-82: LGTM! Idiomatic refactor to type switch.The change from a type assertion with if-statement to a type switch is more idiomatic Go and improves readability while preserving the existing quit behavior.
README.md (1)
5-5: Version bump looks good.Docker image badge correctly updated to v1.8.0, consistent with the PR objectives.
cmd/utils/validateargs.go (1)
45-69: LGTM! Validators follow established patterns.Both
ValidateBerryArgsandValidateCardArgsare implemented consistently with existing validators in the file, properly utilizing the shared helper functions.cmd/card/cardinfo_test.go (1)
46-194: LGTM! Comprehensive test coverage for image handling.The tests for
resizeImageandCardImageare well-structured, covering success paths, decode errors, and HTTP error responses usinghttptestservers. The Sixel output validation is appropriate.cmd/card/cardinfo.go (1)
17-21: LGTM! Clean image resizing implementation.Using
draw.CatmullRom.Scaleprovides good quality for card image resizing..goreleaser.yml (1)
17-17: Version bump is consistent.The ldflags update to v1.8.0 aligns with the PR objectives and version bumps in Dockerfile and nfpm.yaml.
nfpm.yaml (1)
4-4: Package version correctly updated.The version bump to v1.8.0 is consistent with the release tooling in .goreleaser.yml and Dockerfile.
Dockerfile (1)
11-11: Docker image version synchronized.The ldflags update to v1.8.0 completes the coordinated version bump across all build and release artifacts.
.github/workflows/ci.yml (1)
33-33: LGTM!Version bump to v1.8.0 is consistent with the PR title and objectives.
testdata/cli_help.golden (1)
17-17: LGTM!The card command is properly integrated into the help output with a clear description.
cmd/utils/validateargs_test.go (1)
225-262: LGTM!The test follows the established pattern for argument validation tests and correctly exercises the ValidateCardArgs function for valid inputs, invalid inputs, and edge cases.
cmd/card/card_test.go (1)
9-54: LGTM!The test properly exercises the CardCommand function, including help flag handling and error cases. The pattern of temporarily overriding
os.Argsis standard for testing CLI commands.cmd/card/imageviewer_test.go (1)
9-96: LGTM!Comprehensive test coverage for the ImageModel lifecycle, including initialization, key handling (ESC, Ctrl+C, and other keys), and view rendering with various states.
cli.go (1)
12-12: LGTM!The card command is properly integrated into the CLI following the established pattern. All necessary touchpoints (import, help text, command map, error messages) are updated consistently.
Also applies to: 75-75, 112-112, 153-153
cmd/card/cardlist_test.go (1)
11-256: LGTM! Excellent test coverage.The test suite comprehensively covers the CardsModel lifecycle:
- Initialization and state management
- User input handling (ESC, Ctrl+C, Space, navigation)
- View rendering in multiple states (quitting, normal, with/without price data)
The tests are well-structured with clear assertions and good scenario coverage.
cmd/card/serieslist_test.go (1)
1-144: Well-structured test coverage for SeriesModel.The tests comprehensively cover the SeriesModel lifecycle including initialization, quit behavior (both Ctrl+C and Esc), item selection with navigation, window resize handling, and view rendering across different states. Good use of
teatestfor Bubble Tea model testing.cmd/card/setslist_test.go (1)
1-188: Comprehensive test coverage for SetsModel.The tests cover all key interactions: initialization, keyboard events (Esc, Ctrl+C, Enter), window resize handling, and view rendering in different states. The test setup correctly initializes the
setsIDMapfor the Enter key test.cmd/card/serieslist.go (1)
1-81: Clean implementation of the SeriesModel.The model correctly handles key events, maintains state for both display name (
Choice) and underlying ID (SeriesID), and renders appropriate views for different states. The factory function properly configures the list with styling.cmd/card/cardlist.go (1)
157-158: API key is hardcoded—verify this is intentional.Static analysis flagged this as a potential API key exposure. The key prefix
sb_publishable_suggests it's intended to be public (similar to Supabase anon keys), but please confirm this is expected and that the key has appropriate row-level security or rate limiting configured on the backend.go.mod (1)
6-6: Confirm the use of the pre-releasebubblespseudo-version is intentional.The latest stable release of bubbles is v0.21.0, but go.mod pins a pseudo-version (
v0.21.1-0.20250623103423-23b8fd6302d7) instead. If you're targeting unreleased features or fixes, document why the stable release won't suffice. Otherwise, consider updating to the stable release.cmd/card/card.go (3)
1-12: LGTM!Package declaration and imports are appropriate for the TUI-based card command implementation.
37-41: LGTM!Argument validation properly delegates to the utility function and correctly returns errors to the caller.
102-103: LGTM!Return statement correctly returns the accumulated output and nil error for successful execution.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
193-197: Fix nested list indentation.The nested list items use 4-space indentation, but the markdown linter expects 2 spaces for consistency with the rest of the document.
Based on static analysis hints, apply this diff to fix indentation:
- [ ] `card`: get data about a TCG card. - - [x] add mega evolution data - - [x] add scarlet & violet data - - [ ] add sword & shield data - - [ ] add sun & moon data - - [ ] add x & y data + - [x] add mega evolution data + - [x] add scarlet & violet data + - [ ] add sword & shield data + - [ ] add sun & moon data + - [ ] add x & y data
🧹 Nitpick comments (4)
README.md (1)
219-219: Consider adding a blank line before the table.Markdown linters typically expect a blank line before tables for better rendering compatibility across different parsers.
Apply this diff to add a blank line:
--- ## Tested Terminals + | Terminal | OS | Status | Issues |cmd/card/serieslist.go (1)
32-38: Consider adding a defensive check for map access.While the current code is safe since all list items exist in
seriesIDMap, adding a defensive check would improve maintainability and prevent silent failures if the items list and map get out of sync in the future.Apply this diff for defensive coding:
case "enter": i, ok := m.List.SelectedItem().(item) if ok { m.Choice = string(i) - m.SeriesID = seriesIDMap[string(i)] + if id, exists := seriesIDMap[string(i)]; exists { + m.SeriesID = id + } } return m, tea.Quitdocs/Infrastructure_Guide/index.md (1)
24-36: Minor wording tweak in step 7Very clear description overall. For step 7, you could tighten the phrasing to:
Users can then query the
pokeapi.coorsupabaseAPIs for either video game or trading card data, respectively.Purely stylistic; feel free to ignore if you prefer the current wording.
card_data/pipelines/defs/load/load_pricing_data.py (1)
31-61: Soda asset wiring is good; consider tightening subprocess and exception handlingThe
data_quality_checks_on_pricingasset is wired cleanly as a Soda “post‑load” step (deps=[load_pricing_data], explicit name matching the translator mapping), and the relative paths look correct fromdefs/loadinto thesodadirectory.If you want to address the Ruff hints and harden things a bit, consider:
- Executable resolution (S607): Resolve the
sodabinary to an absolute path up front so failures are clearer and PATH spoofing is less of a concern:+import shutil @@ -def data_quality_checks_on_pricing() -> None: - current_file_dir = Path(__file__).parent - print(f"Setting cwd to: {current_file_dir}") +def data_quality_checks_on_pricing() -> None: + current_file_dir = Path(__file__).parent + print(f"Setting cwd to: {current_file_dir}") + + soda_executable = shutil.which("soda") + if not soda_executable: + raise RuntimeError("soda CLI not found on PATH; cannot run pricing data quality checks.") @@ - result = subprocess.run( - [ - "soda", + result = subprocess.run( + [ + soda_executable, "scan", @@ - if result.returncode != 0: - raise Exception(f"Soda data quality checks failed with return code {result.returncode}") + if result.returncode != 0: + raise RuntimeError(f"Soda data quality checks failed with return code {result.returncode}")
- Exception type (TRY002/TRY003): Using
RuntimeError(or a small customSodaScanError) keeps the failure explicit without relying on a bareException.Optional: if you later touch this file again, switching from
dg.get_dagster_logger()) would make output integrate better with Dagster logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/assets/data_infrastructure_diagram.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
README.md(6 hunks)card_data/pipelines/definitions.py(3 hunks)card_data/pipelines/defs/load/load_pricing_data.py(2 hunks)card_data/pipelines/defs/transformation/transform_data.py(1 hunks)card_data/pipelines/soda/checks_pricing.yml(1 hunks)cmd/card/serieslist.go(1 hunks)docs/Infrastructure_Guide/index.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
card_data/pipelines/definitions.py (2)
card_data/pipelines/defs/load/load_pricing_data.py (2)
load_pricing_data(17-28)data_quality_checks_on_pricing(36-61)card_data/pipelines/defs/extract/extract_pricing_data.py (1)
build_dataframe(111-127)
🪛 LanguageTool
README.md
[uncategorized] ~226-~226: The name of this game or TV show is spelled with an accent.
Context: ... | - Some colors aren't supported.
- pokemon <name> --image=xx flag pixel issues. ...
(POKEMON)
[uncategorized] ~227-~227: The name of this game or TV show is spelled with an accent.
Context: ... not support sixel for TCG images.
- pokemon <name> --image=xx flag pixel issues. |...
(POKEMON)
docs/Infrastructure_Guide/index.md
[style] ~35-~35: As a shorter alternative for ‘able to’, consider using “can then”.
Context: ... a Supabase production schema. 7. Users are then able to query the pokeapi.co or supabase AP...
(BE_ABLE_TO)
🪛 markdownlint-cli2 (0.18.1)
README.md
32-32: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
193-193: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
195-195: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
196-196: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
197-197: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
219-219: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Ruff (0.14.5)
card_data/pipelines/defs/load/load_pricing_data.py
41-49: Starting a process with a partial executable path
(S607)
61-61: Create your own exception
(TRY002)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
README.md (3)
5-5: LGTM!Version references are consistently updated to v1.8.0 across the Docker badge and installation commands.
Also applies to: 98-98, 102-102
18-32: LGTM!The Demo section is well-structured with clear subsections for Video Game Data and Trading Card Game Data, each with updated demonstration images that showcase the new card functionality.
166-166: LGTM!The new
cardcommand is correctly added to the COMMANDS section, aligning with the new Trading Card Game data feature introduced in this release.cmd/card/serieslist.go (3)
1-6: LGTM!Package declaration and imports are correctly structured for a Bubble Tea UI component.
14-23: LGTM!The
SeriesModelstruct is well-defined with appropriate fields for managing the series selection UI state, and theInit()method correctly returns nil since no initialization command is needed.
51-80: LGTM!The
View()method correctly handles conditional rendering based on the model state, andSeriesList()properly initializes the Bubble Tea list model with appropriate configuration.card_data/pipelines/defs/transformation/transform_data.py (1)
13-21: dbt source → asset mapping forpricing_datalooks consistentRouting the
pricing_datasource throughdata_quality_checks_on_pricing(instead ofload_pricing_data) nicely inserts the Soda checks into the dependency chain while still depending on the load asset. As long as the dbt source name remains"pricing_data"and the Dagster asset name stays"data_quality_checks_on_pricing", this wiring should behave as intended.card_data/pipelines/definitions.py (1)
18-21: Pricing pipeline selection now scales better with downstream assetsUsing
AssetSelection.assets(build_dataframe).downstream(include_self=True)is a nice improvement over manually listing specific assets: it will automatically pick upload_pricing_data,data_quality_checks_on_pricing, and any future downstream dbt assets. Includingdata_quality_checks_on_pricingindefs_pricing.assetsalso keeps the asset catalog complete.Also applies to: 30-32
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cmd/card/imageviewer_test.go (3)
47-60: Good coverage for Ctrl+C quit pathThis test mirrors the ESC case and ensures Ctrl+C also triggers a quit command, which is useful for regressions. If you later consolidate behavior for multiple quit keys, you could convert ESC/Ctrl+C into a small table-driven test, but it’s fine as-is.
62-74: Non-quit key path is well coveredThe negative test around a non-quit key returning
nilcommand is clear and keeps the event loop from doing unexpected work. If you want to tighten it further, you could also assert that the returned model is unchanged, but that’s optional.
26-45: Now let me verify the actual code in the repository to ensure the review comment is addressing real code:Based on my verification using the web search results you provided and additional research on Bubble Tea testing patterns:
Strengthen ESC handling assertion to verify actual quit behavior
The test checks
cmd != nil, which would pass for any returned command, not specificallytea.Quit. Sincetea.QuitMsgsignals that the program should quit and can be sent withQuit, your suggestion is well-founded. As QuitMsg signals program quit, a more rigorous test would execute the command and assert that it actually returns atea.QuitMsg, rather than just verifying a command exists. This would prevent accidentally returning a differenttea.Cmdfrom being missed.Example assertion:
// After: newModel, cmd := model.Update(msg) if cmd == nil { t.Error("Update with ESC should return tea.Quit command") } if msg, ok := cmd().(tea.QuitMsg); !ok { t.Error("Update should return tea.Quit, not another command") }cmd/card/cardinfo_test.go (2)
72-112: Test covers the success path adequately.The mock server approach and Sixel validation are appropriate. Minor note: the
len(result) == 0check on line 109-111 is redundant since ifHasPrefixandHasSuffixboth pass, the string cannot be empty.Consider removing the redundant check:
if !strings.HasSuffix(result, "\x1b\\") { t.Error("CardImage() should return string ending with Sixel terminator") } - - if len(result) == 0 { - t.Error("CardImage() should return non-empty string") - }
86-89: Silent failure on encode error masks test failures.If
png.Encodefails, the handler returns silently with a partial response. This could cause confusing test failures. Consider failing the test explicitly.err := png.Encode(w, img) if err != nil { + t.Fatalf("failed to encode test PNG: %v", err) - return }Note: You'll need to pass
tinto the handler or usehttp.Error(w, ...)to surface the issue.cmd/card/card.go (1)
29-35: Global flag state may interfere with subcommands.Using the global
flag.Parse()and checkingos.Argsdirectly can cause issues if this command is called multiple times or if flags are defined elsewhere. Consider using a dedicatedflag.FlagSetfor this subcommand.+ cardFlags := flag.NewFlagSet("card", flag.ContinueOnError) + cardFlags.Usage = func() { helpMessage := styling.HelpBorder.Render( // ... ) output.WriteString(helpMessage) } - - flag.Parse() + if err := cardFlags.Parse(os.Args[2:]); err != nil { + return output.String(), nil + }cmd/card/cardlist.go (1)
28-30: Consider initializing SelectedOption on Init.
Init()returns nil, soSelectedOptionremains empty until the first table navigation. This could cause issues if the image viewer is triggered before any navigation occurs.Consider syncing the initial selection in
Init()orCardsList():func CardsList(setID string) (CardsModel, error) { // ...existing code... + initialSelection := "" + if len(rows) > 0 { + initialSelection = rows[0][0] + } return CardsModel{ IllustratorMap: illustratorMap, ImageMap: imageMap, PriceMap: priceMap, + SelectedOption: initialSelection, Table: t, }, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitleaksignore(1 hunks)cmd/card/card.go(1 hunks)cmd/card/cardinfo_test.go(1 hunks)cmd/card/cardlist.go(1 hunks)cmd/card/imageviewer.go(1 hunks)cmd/card/imageviewer_test.go(1 hunks)cmd/card/setslist.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitleaksignore
- cmd/card/setslist.go
- cmd/card/imageviewer.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/card/cardinfo_test.go (1)
cmd/card/cardinfo.go (1)
CardImage(23-58)
cmd/card/cardlist.go (1)
styling/styling.go (3)
TypesTableBorder(42-44)Color(84-86)KeyMenu(22-22)
🔇 Additional comments (14)
cmd/card/imageviewer_test.go (6)
14-24: Init test correctly verifies no startup commandThis cleanly asserts that
Initdoes not schedule any initial command, which matches a simple, synchronous viewer model. Looks good as-is.
76-88: View test correctly ties output to ImageURLAsserting that
View()simply returns the underlyingImageURLmakes the model/view contract explicit and easy to refactor against. No issues here.
90-101: Empty View behavior is explicitly specifiedVerifying that an empty
ImageURLyields an empty view string gives a clear contract for “no image” scenarios. This should help avoid accidental placeholder text or formatting changes. Looks good.
103-133: Robust success-path test with an in-memory PNGNice use of
httptest.NewServerand a real PNG generated viaimage/pngto exercise the happy path. The checks forCardName,Error == nil, and non-emptyImageURLtogether give good confidence that fetching, decoding, and transforming the image succeeded.
135-155: HTTP error path is clearly validatedThe 404 test neatly asserts that
CardNameis still set whileErroris non-nil andImageURLremains empty. This is a solid contract for surfacing fetch failures to the caller.
157-179: Invalid-image data path is well coveredThis test ensures that decoding failures (bad image bytes with an image content type) set
Errorand leaveImageURLempty, while preservingCardName. Together with the 404 test, it gives good coverage of the main error modes.cmd/card/cardinfo_test.go (3)
13-70: Well-structured table-driven test for resizeImage.The test properly exercises the
resizeImagefunction with multiple dimension scenarios including smaller, larger, and card-specific dimensions. The test image creation and assertions are correct.
114-139: Good error path coverage for decoding failures.The test correctly validates that invalid image data returns an appropriate error message.
141-161: Good error path coverage for non-200 responses.The test validates HTTP status code handling correctly.
cmd/card/card.go (2)
43-53: Previous issues addressed: proper error handling and safe type assertions.The code now correctly returns errors instead of calling
os.Exit(1)and uses safe type assertions withokchecks, addressing the concerns from previous reviews.
79-105: Previous type assertion issue addressed; image viewer error handling is reasonable.The safe type assertion on line 85-88 addresses the previous review concern. The warning-only approach for image viewer errors (line 96) is acceptable since image viewing is non-critical functionality.
cmd/card/cardlist.go (3)
152-179: Previous issues addressed: timeout and status validation added.The HTTP client now has a 15-second timeout (line 162) and validates the response status code (lines 169-171), addressing the concerns from the previous review.
32-58: Update method correctly handles navigation and state sync.The keyboard handling for quit and view image, along with the selected option synchronization, is well implemented.
60-91: View method handles display states appropriately.The left/right panel layout with proper fallback for missing price data is well structured.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.