1.8.3 #221
1.8.3 #221
Conversation
📝 WalkthroughWalkthroughAdds a gitleaks gate to CI, upgrades several GitHub Action versions, bumps project version to v1.8.3, restructures card_data extract/load into tcgdex/tcgcsv modules with Soda checks and utilities, refactors Go flag APIs to struct wrappers, updates command error handling, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (push/PR)
participant GH as GitHub Actions
participant Gitleaks as Gitleaks Action
participant Build as build-linux-packages
participant Upload as Cloudsmith Upload
Dev->>GH: push / open PR
GH->>Gitleaks: checkout (fetch-depth=0) & run gitleaks (uses gitleaks.toml)
alt gitleaks succeeds
Gitleaks-->>GH: success
GH->>Build: needs.gitleaks → checkout & build packages
Build-->>GH: produce artifacts
GH->>Upload: upload artifacts to Cloudsmith
Upload-->>GH: upload result
else gitleaks fails
Gitleaks-->>GH: failure → subsequent jobs gated/blocked
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/berry/berry.go (1)
120-130: Inconsistent error handling:log.Fatalfstill exits the process.The function signature was updated to return an error, but line 129 still uses
log.Fatalfwhich will terminate the process. This bypasses the new error propagation pattern and makes the function harder to test.🔎 Proposed fix
namesList, err := connections.QueryBerryData(` SELECT UPPER(SUBSTR(name, 1, 1)) || SUBSTR(name, 2) FROM berries ORDER BY name`) if err != nil { - log.Fatalf("Failed to get berry names: %v", err) + return fmt.Errorf("failed to get berry names: %w", err) }
🧹 Nitpick comments (15)
cmd/berry/berry_test.go (1)
197-253: LGTM! Comprehensive navigation and quit behavior tests.The test logic correctly verifies:
- Navigation sequence (down, down, up → "Aspear")
- Escape key quitting
- Default initial selection
Consider extracting repeated test constants to improve maintainability:
🔎 Optional: Extract common test configuration
+const ( + testTermWidth = 100 + testTermHeight = 50 + testTimeout = 300 * time.Millisecond +) + func TestTableNavigation(t *testing.T) { m := createTestModel() - testModel := teatest.NewTestModel(t, m, teatest.WithInitialTermSize(100, 50)) + testModel := teatest.NewTestModel(t, m, teatest.WithInitialTermSize(testTermWidth, testTermHeight)) // Navigate down twice testModel.Send(tea.KeyMsg{Type: tea.KeyDown}) testModel.Send(tea.KeyMsg{Type: tea.KeyDown}) // Navigate back up once testModel.Send(tea.KeyMsg{Type: tea.KeyUp}) // Quit the program testModel.Send(tea.KeyMsg{Type: tea.KeyCtrlC}) - testModel.WaitFinished(t, teatest.WithFinalTimeout(300*time.Millisecond)) + testModel.WaitFinished(t, teatest.WithFinalTimeout(testTimeout))cmd/types/types_test.go (1)
163-179: LGTM! Test correctly verifies type selection navigation.The navigation sequence (down, down, enter) correctly results in selecting "Water" (the third type in the list).
Consider removing the
Ctrl+CafterEnter, asEnteralready triggerstea.Quitin the Update handler (line 78 of types.go):🔎 Optional: Simplify test by removing redundant quit signal
testModel.Send(tea.KeyMsg{Type: tea.KeyDown}) testModel.Send(tea.KeyMsg{Type: tea.KeyDown}) testModel.Send(tea.KeyMsg{Type: tea.KeyEnter}) - testModel.Send(tea.KeyMsg{Type: tea.KeyCtrlC}) testModel.WaitFinished(t, teatest.WithFinalTimeout(300*time.Millisecond))flags/abilityflagset.go (1)
69-71: No-op statement:fmt.Fprint(w)with no content.This call writes nothing to the writer and serves no purpose. Consider removing it or replacing with the intended output.
🔎 Proposed fix
- if _, err := fmt.Fprint(w); err != nil { - return err - } - return nilflags/pokemonflagset_test.go (1)
16-37: LGTM with minor note on test descriptions.Test correctly adapted to the new composite
PokemonFlagsstruct. However, lines 30-31 have misleading test names stating "default value should be 'md'" while the actual expected value is an empty string"". Consider updating the descriptions:- {pf.Image, "", "Image flag default value should be 'md'"}, - {pf.ShortImage, "", "Short image flag default value should be 'md'"}, + {pf.Image, "", "Image flag default value should be empty"}, + {pf.ShortImage, "", "Short image flag default value should be empty"},flags/pokemonflagset.go (1)
390-401: Consider removingfmt.Printlncalls since errors are now returned.The function now returns errors instead of exiting, which is good. However, lines 392 and 399 still print to stdout before returning. This could cause duplicate error messages if the caller also logs or displays the returned error.
🔎 Proposed fix
imageResp, err := http.Get(pokemonStruct.Sprites.FrontDefault) if err != nil { - fmt.Println("Error downloading sprite image:", err) - return err + return fmt.Errorf("error downloading sprite image: %w", err) } defer imageResp.Body.Close() img, err := imaging.Decode(imageResp.Body) if err != nil { - fmt.Println("Error decoding image:", err) - return err + return fmt.Errorf("error decoding image: %w", err) }README.md (1)
14-15: Consider consistent spelling of Pokémon.The heading uses "Pokemon" while the description uses "Pokémon" (with accent). For consistency, consider using "Pokémon" throughout or documenting the intentional choice (e.g., ASCII-safe headings for compatibility).
card_data/pipelines/utils/json_retriever.py (1)
4-8: Consider more precise type hints and add a docstring.The return type
dictis not accurate—response.json()can return a list, dict, or other JSON-compatible types. Consider usingAnyor a more specific Union type. Additionally, a docstring would clarify the function's purpose, parameters, and exceptions.🔎 Proposed improvements
+from typing import Any + import requests -def fetch_json(url: str, timeout: int = 30) -> dict: +def fetch_json(url: str, timeout: int = 30) -> Any: + """ + Fetch JSON data from a URL. + + Args: + url: The URL to fetch from + timeout: Request timeout in seconds (default: 30) + + Returns: + Parsed JSON response (dict, list, or other JSON-compatible type) + + Raises: + requests.RequestException: If the request fails + requests.HTTPError: If the response status is not 2xx + requests.JSONDecodeError: If response is not valid JSON + """ response = requests.get(url, timeout=timeout) response.raise_for_status() return response.json()docs/Infrastructure_Guide/index.md (1)
53-89: Consider adding language specifier to code block.The code block would benefit from a language specifier (e.g.,
textortree) for proper syntax highlighting and to satisfy markdown linting rules.🔎 Proposed fix
-``` +```text . ├── pipelines/card_data/pipelines/defs/extract/tcgdex/extract_cards.py (1)
100-109: Consider redundant attack data storage.Attack data is stored in two forms:
- As serialized JSON in
attacks_json(Line 100)- As flattened columns
attack_1_name,attack_1_damage, etc. (Lines 102-109)This duplication increases storage and maintenance burden. Consider whether both representations are necessary, or if one can be derived from the other downstream.
card_data/pipelines/utils/secret_retriever.py (1)
6-10: Remove unnecessary cast —get_secret_stringalready returnsstr.The
SupabaseSecretTypedDict and type annotations are well-implemented and enhance code maintainability. However, thecast(str, ...)on line 18 is redundant. The AWS Secrets Manager Caching library'sSecretCache.get_secret_string()method already returnsstr, notOptional[str], so the explicit cast doesn't add type safety and can be removed for cleaner code.card_data/pipelines/defs/load/tcgdex/load_cards.py (2)
18-18: Remove unnecessary variable assignment.The intermediate variable
dfis redundant and can be eliminated for cleaner code.🔎 Proposed fix
- df = create_card_dataframe try: - df.write_database( + create_card_dataframe.write_database( table_name=table_name, connection=database_url, if_table_exists="append" )
14-26: Consider adding data quality checks for consistency.Unlike
load_sets.pyandload_series.py, this module doesn't include a data quality check asset (similar todata_quality_check_on_seriesordata_quality_check_on_sets). If data quality validation is needed for cards data, consider adding a similar Soda-based check asset for consistency across the pipeline.Do you want me to generate a data quality check asset similar to the ones in
load_sets.pyandload_series.py?card_data/pipelines/defs/load/tcgdex/load_sets.py (1)
16-33: LGTM! Asset implementation is solid.The retry policy and error handling are correctly configured.
Consider removing the unnecessary intermediate variable on line 25:
- df = extract_sets_data try: - df.write_database( + extract_sets_data.write_database( table_name=table_name, connection=database_url, if_table_exists="replace" )card_data/pipelines/defs/load/tcgdex/load_series.py (1)
16-33: LGTM! Asset implementation is correct.The retry policy, error handling, and database write configuration are appropriate.
Consider removing the unnecessary intermediate variable on line 25:
- df = extract_series_data try: - df.write_database( + extract_series_data.write_database( table_name=table_name, connection=database_url, if_table_exists="replace" )docs/Infrastructure_Guide/local-deployment.md (1)
51-51: Consider fixing markdown code block formatting.Static analysis flagged several code blocks that should use fenced format (triple backticks with language identifier) instead of indented format for better rendering and syntax highlighting.
Based on learnings, this is a minor documentation formatting improvement that can be addressed in a follow-up if needed.
Also applies to: 71-71, 119-119, 190-190, 225-225
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
card_data/uv.lockis excluded by!**/*.lockdocs/assets/dagster_ui.pngis excluded by!**/*.pngpoke-cli.pngis excluded by!**/*.png
📒 Files selected for processing (51)
.github/workflows/ci.yml.github/workflows/coverage.yml.github/workflows/go_lint.yml.github/workflows/go_test.yml.github/workflows/release.yml.gitignore.gitleaksignore.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/definitions.pycard_data/pipelines/defs/extract/extract_data.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/defs/load/load_data.pycard_data/pipelines/defs/load/tcgcsv/load_pricing.pycard_data/pipelines/defs/load/tcgdex/load_cards.pycard_data/pipelines/defs/load/tcgdex/load_series.pycard_data/pipelines/defs/load/tcgdex/load_sets.pycard_data/pipelines/defs/transform/transform_data.pycard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pipelines/poke_cli_dbt/models/sets.sqlcard_data/pipelines/soda/checks_series.ymlcard_data/pipelines/soda/checks_sets.ymlcard_data/pipelines/utils/json_retriever.pycard_data/pipelines/utils/secret_retriever.pycard_data/pyproject.tomlcli.gocli_test.gocmd/ability/ability.gocmd/berry/berry.gocmd/berry/berry_test.gocmd/card/cardlist_test.gocmd/pokemon/pokemon.gocmd/search/model_selection_test.gocmd/search/search.gocmd/search/search_test.gocmd/types/types.gocmd/types/types_test.godocs/Infrastructure_Guide/cloud-deployment.mddocs/Infrastructure_Guide/index.mddocs/Infrastructure_Guide/local-deployment.mdflags/abilityflagset.goflags/abilityflagset_test.goflags/pokemonflagset.goflags/pokemonflagset_test.gogitleaks.tomlnfpm.yamltestdata/main_latest_flag.golden
💤 Files with no reviewable changes (2)
- card_data/pipelines/defs/load/load_data.py
- card_data/pipelines/defs/extract/extract_data.py
🧰 Additional context used
🧬 Code graph analysis (18)
cmd/search/search.go (1)
cmd/utils/validateargs.go (1)
ValidateSearchArgs(181-191)
cmd/types/types.go (1)
cmd/types/damage_table.go (1)
DamageTable(17-100)
card_data/pipelines/defs/extract/tcgdex/extract_cards.py (1)
card_data/pipelines/utils/json_retriever.py (1)
fetch_json(4-8)
flags/pokemonflagset.go (1)
styling/styling.go (1)
StyleItalic(28-28)
card_data/pipelines/definitions.py (2)
card_data/pipelines/defs/extract/tcgcsv/extract_pricing.py (1)
build_dataframe(159-177)card_data/pipelines/defs/load/tcgcsv/load_pricing.py (2)
load_pricing_data(17-28)data_quality_checks_on_pricing(36-61)
cmd/card/cardlist_test.go (1)
cmd/card/cardlist.go (1)
CallCardData(165-192)
cmd/search/search_test.go (1)
cmd/search/search.go (1)
SearchCommand(14-47)
cmd/ability/ability.go (1)
flags/abilityflagset.go (1)
SetupAbilityFlagSet(21-37)
flags/abilityflagset.go (1)
styling/styling.go (2)
HelpBorder(30-32)StyleBold(27-27)
card_data/pipelines/defs/load/tcgdex/load_sets.py (3)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_secret(13-23)card_data/pipelines/defs/extract/tcgdex/extract_sets.py (1)
extract_sets_data(22-56)card_data/pipelines/defs/load/tcgdex/load_series.py (1)
data_quality_check_on_series(37-65)
flags/pokemonflagset_test.go (1)
flags/pokemonflagset.go (1)
SetupPokemonFlagSet(58-98)
card_data/pipelines/defs/load/tcgdex/load_series.py (3)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_secret(13-23)card_data/pipelines/defs/extract/tcgdex/extract_series.py (1)
extract_series_data(18-36)card_data/pipelines/defs/load/tcgdex/load_sets.py (1)
data_quality_check_on_series(37-65)
card_data/pipelines/defs/extract/tcgdex/extract_series.py (1)
card_data/pipelines/utils/json_retriever.py (1)
fetch_json(4-8)
cmd/berry/berry_test.go (1)
styling/styling.go (1)
Color(83-85)
card_data/pipelines/defs/extract/tcgdex/extract_sets.py (1)
card_data/pipelines/utils/json_retriever.py (1)
fetch_json(4-8)
card_data/pipelines/defs/load/tcgcsv/load_pricing.py (1)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_secret(13-23)
cmd/pokemon/pokemon.go (1)
flags/pokemonflagset.go (6)
SetupPokemonFlagSet(58-98)AbilitiesFlag(100-151)DefenseFlag(153-335)MovesFlag(426-569)StatsFlag(571-664)TypesFlag(666-696)
flags/abilityflagset_test.go (1)
flags/abilityflagset.go (1)
SetupAbilityFlagSet(21-37)
🪛 LanguageTool
docs/Infrastructure_Guide/index.md
[style] ~90-~90: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1495 characters long)
Context: ...├── pyproject.toml └── uv.lock ``` !!! note This project is a learning pl...
(EN_EXCESSIVE_EXCLAMATION)
docs/Infrastructure_Guide/local-deployment.md
[style] ~187-~187: Using many exclamation marks might seem excessive (in this case: 12 exclamation marks for a text that’s 4658 characters long)
Context: ...om the staging schema. --- ## Soda !!! question "What is Soda?" Soda is a...
(EN_EXCESSIVE_EXCLAMATION)
README.md
[uncategorized] ~14-~14: The name of this game or TV show is spelled with an accent.
Context: ...olor=00ADD8" alt="codecov"/> ## Pokemon CLI poke-cli is a hybrid of a classic...
(POKEMON)
[uncategorized] ~15-~15: Do not mix variants of the same word (‘Pokémon’ and ‘Pokemon’) within a single text.
Context: ... tool for viewing VG and TCG data about Pokémon! View the [documentation](https://docs...
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.18.1)
docs/Infrastructure_Guide/index.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
docs/Infrastructure_Guide/local-deployment.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
71-71: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
119-119: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
190-190: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
225-225: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🪛 Ruff (0.14.10)
card_data/pipelines/defs/load/tcgdex/load_sets.py
41-41: subprocess call: check for execution of untrusted input
(S603)
63-65: Create your own exception
(TRY002)
63-65: Avoid specifying long messages outside the exception class
(TRY003)
card_data/pipelines/defs/load/tcgdex/load_series.py
41-41: subprocess call: check for execution of untrusted input
(S603)
63-65: Create your own exception
(TRY002)
63-65: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (64)
.gitignore (1)
69-69: LGTM!Adding
REFACTORING.mdto the ignore list is appropriate and aligns with preventing transient refactoring documentation from being tracked in version control.card_data/pyproject.toml (2)
62-63: LGTM! Reasonable bandit exclusions.The bandit configuration appropriately excludes non-production directories (virtual environments, logs, storage, and sample scripts) from security scanning. This aligns with the PR's focus on enhancing security scanning infrastructure.
53-60: No changes needed. The**/*.py*pattern is the documented default for pyrefly'sproject-includesoption, and the configuration is correctly placed in[tool.pyrefly]section ofpyproject.tomlper official pyrefly documentation. The pattern intentionally includes Python source files and related extensions like compiled bytecode, which is the tool's expected behavior.Likely an incorrect or invalid review comment.
cmd/berry/berry_test.go (2)
7-12: LGTM! Test dependencies added appropriately.The new imports support the Bubble Tea UI testing infrastructure using
teatestfor simulating terminal interactions.
167-195: LGTM! Well-structured test helper.The
createTestModelfunction provides isolated test data without requiring database calls, which is a good testing practice.cmd/types/types.go (3)
46-49: LGTM! Improved error propagation.The refactor to return errors instead of exiting improves testability and allows callers to handle failures appropriately.
104-132: LGTM! Clean separation of table creation logic.Extracting the table creation into a dedicated function improves testability and follows the single responsibility principle.
134-147: LGTM! Well-structured program execution with proper error handling.The function correctly:
- Separates model creation from execution
- Wraps errors for proper error chain propagation
- Only invokes
DamageTablewhen a valid selection is madecmd/search/model_selection_test.go (2)
20-20: LGTM! Added explicit timeout for test stability.The explicit timeout prevents tests from hanging indefinitely if the program doesn't terminate as expected.
42-51: LGTM! Improved comment clarity.The updated comments better describe the test intentions and expected behavior.
cli.go (1)
120-125: LGTM! Proper error handling for SearchCommand.The error handling correctly:
- Captures the error from
SearchCommand()- Displays a styled error message to the user
- Returns appropriate exit codes (1 for error, 0 for success)
This aligns with the refactored
SearchCommand()signature that now returns errors instead of exiting.cmd/search/search.go (2)
14-14: LGTM! Function signature updated to return error.This change enables proper error propagation to the caller (cli.go), improving testability and allowing graceful error handling.
32-46: LGTM! Consistent error handling throughout the function.All error paths correctly:
- Return the error instead of exiting
- Return nil on success (including help display)
- Maintain existing error logging for user visibility
cmd/ability/ability.go (3)
34-34: LGTM! Centralized flag management with struct-based approach.The refactor to use
flags.SetupAbilityFlagSet()improves code organization by consolidating flag setup logic into a reusable structure.
53-58: LGTM! Flag parsing updated to use AbilityFlags struct.The change to
af.FlagSet.Parse()andaf.FlagSet.Usage()correctly aligns with the new struct-based flag management pattern.
103-108: LGTM! Flag references updated to use struct fields.The conditional now correctly references
af.Pokemonandaf.ShortPokemonfrom theAbilityFlagsstruct.cmd/search/search_test.go (1)
90-95: LGTM! Test updated to verify error return behavior.The test now correctly:
- Captures the error return value from
SearchCommand()- Asserts based on the
expectedErrorfield usingassert.Errororassert.NoErrorThis aligns with the updated
SearchCommand()signature that now returns an error.cmd/berry/berry.go (2)
47-50: LGTM!Good refactor to propagate errors from
tableGeneration()instead of exiting the process. This improves testability and allows callers to handle errors appropriately.
157-161: LGTM!Good use of
fmt.Errorfwith%wfor error wrapping, enabling error unwrapping by callers.flags/abilityflagset.go (1)
15-36: LGTM!Good refactor to consolidate flag variables into a struct. This approach improves code organization and makes the API cleaner by returning a single object instead of multiple values.
flags/abilityflagset_test.go (1)
16-34: LGTM!Test correctly adapted to use the new composite
AbilityFlagsstruct. The assertions properly validate the struct fields and flag set configuration.flags/pokemonflagset.go (2)
27-41: LGTM!Well-structured flag container that consolidates all Pokémon-related flags into a single struct, improving API ergonomics and consistency with the
AbilityFlagspattern.
58-97: LGTM!Clean refactor that returns the consolidated flag struct. The Usage function is properly attached to the FlagSet.
cmd/pokemon/pokemon.go (3)
48-48: LGTM!Correctly adopts the new struct-based flag setup pattern.
68-72: LGTM!Good improvement to return errors instead of calling
os.Exit(1). This makes the function more testable and allows callers to handle errors appropriately.
202-224: LGTM!Flag checks correctly updated to use the struct fields (
pf.Image,pf.ShortImage,pf.Abilities, etc.). The dereferencing of pointer fields is consistent throughout.nfpm.yaml (1)
4-4: LGTM! Version bump is consistent with the release.The version update to v1.8.3 aligns correctly with the PR objectives and other version bumps across the repository.
Dockerfile (1)
11-11: LGTM! Docker build version updated correctly.The version embedded in the binary via ldflags is properly updated to v1.8.3, consistent with other release artifacts.
.goreleaser.yml (1)
17-17: LGTM! GoReleaser version updated correctly.The version string in ldflags is properly updated to v1.8.3, ensuring all release binaries will report the correct version.
card_data/pipelines/poke_cli_dbt/dbt_project.yml (2)
2-2: LGTM! Version bump is consistent.The dbt project version is correctly updated to 1.8.3, aligning with the broader release.
18-21: LGTM! Explicit materialization configuration improves clarity.Making the materialization strategy explicit at the model level is a good practice for dbt projects, ensuring all models under
poke_cli_dbtconsistently materialize as tables.card_data/pipelines/soda/checks_series.yml (2)
24-25: LGTM! Minor formatting improvement.The formatting adjustment improves readability without affecting functionality.
3-3: Row count of 3 is correct—the series extraction intentionally filters to three specific series.The
extract_series_data()function inextract_series.pyexplicitly filters the API response to only three series IDs ("swsh","sv","me"), making therow_count = 3expectation correct and intentional. The filtering occurs at extraction time rather than in the SQL models. This aligns with the pipeline's design to focus on these three specific series.testdata/main_latest_flag.golden (1)
1-7: Verify the expected version in the test data.This golden file shows v1.8.2 while the PR bumps the project version to v1.8.3. If this tests the
--latestflag by checking GitHub's latest release, v1.8.2 may be correct (since v1.8.3 isn't released yet). However, if this should reflect the new version, it needs updating.Run the following script to check how this golden file is used in tests:
#!/bin/bash # Description: Find test files that reference this golden file to understand expected behavior # Search for references to main_latest_flag.golden rg -n "main_latest_flag.golden" --type=go -C 5cli_test.go (1)
32-34: Excellent addition of test state preservation.Saving and restoring the original version prevents test pollution and ensures tests don't interfere with each other. This is a best practice for testing code that modifies global state.
cmd/card/cardlist_test.go (1)
305-352: Excellent test coverage for CallCardData.The new tests comprehensively cover:
- Happy path with proper header validation
- Non-200 HTTP error responses
- Invalid URL handling
This significantly improves the reliability of the HTTP data fetching layer.
card_data/pipelines/defs/extract/tcgdex/extract_cards.py (1)
13-13: Hardcoded URL list appears to be a placeholder.The URL list contains only a single hardcoded set URL (
me02). This seems like placeholder/test code that should be replaced with dynamic set discovery or configuration.Is this intentional for testing, or should this be refactored to accept dynamic set IDs (e.g., from configuration or upstream assets)?
.github/workflows/coverage.yml (1)
14-14: LGTM!The checkout action version bump to v6 is consistent with the broader CI/CD workflow updates in this PR.
card_data/pipelines/defs/load/tcgcsv/load_pricing.py (1)
47-48: The Soda configuration file paths at lines 47-48 correctly resolve to the intended files. Bothconfiguration.ymlandchecks_pricing.ymlexist incard_data/pipelines/soda/, and the relative paths../../../soda/correctly navigate from the file's parent directory (card_data/pipelines/defs/load/tcgcsv/) to the Soda directory when the subprocess runs with the configuredcwd..github/workflows/go_lint.yml (1)
18-18: LGTM! Checkout action upgrade is appropriate.The upgrade from
actions/checkout@v4tov6aligns with the broader workflow modernization in this PR..github/workflows/go_test.yml (1)
14-14: LGTM! Consistent with other workflow upgrades.The checkout action upgrade matches the pattern across all workflow files in this PR.
.github/workflows/release.yml (1)
16-16: LGTM! Release workflow upgrade is consistent.The checkout action upgrade maintains consistency across all GitHub Actions workflows.
docs/Infrastructure_Guide/cloud-deployment.md (2)
7-7: LGTM! Documentation clarity improved.The addition of "on a schedule" clarifies the operational context for running Dagster pipelines.
57-57: LGTM! Punctuation improvements enhance readability.The added colons create a more consistent and professional documentation format for step-by-step instructions.
Also applies to: 61-61, 65-65
card_data/pipelines/soda/checks_sets.yml (1)
6-37: LGTM! Comprehensive data quality checks are well-defined.The schema validation, completeness checks, uniqueness constraints, URL format validation, and foreign key checks provide thorough data quality coverage for the sets dataset.
gitleaks.toml (1)
1-10: The allowlist pattern is correct and appropriately protects Supabase publishable keys.The regex pattern
sb_publishable_[A-Za-z0-9_-]+correctly targets only Supabase publishable/anon keys and does not match secret key patterns. The codebase contains Supabase publishable keys that match this pattern, and no secret keys are at risk of being inadvertently allowlisted.card_data/pipelines/defs/load/tcgdex/load_cards.py (1)
1-13: LGTM! Asset configuration is correct.The retry policy and asset configuration are properly set up for reliable data loading.
card_data/pipelines/defs/extract/tcgdex/extract_series.py (3)
1-15: LGTM! Model definition is clean.The Pydantic model with
HttpUrlvalidation for the logo field provides good type safety and validation.
17-31: LGTM! Validation pattern is solid.The Pydantic validation with colored output and proper error re-raising provides clear feedback and maintains error propagation.
33-36: LGTM! Filtering and conversion logic is appropriate.The hardcoded series filter is clear and the DataFrame conversion is properly implemented.
card_data/pipelines/definitions.py (2)
7-8: LGTM! Type annotations improve code quality.The addition of explicit type annotations (
dg.Definitions,dg.ScheduleDefinition) enhances maintainability and provides better IDE support.Also applies to: 12-12, 14-14, 23-23, 30-30
25-25: Verify the cron schedule time change is intentional.The schedule has been updated from
"31 21 * * *"(9:31 PM) to"0 14 * * *"(2:00 PM). This is a significant shift in execution time and may impact when data is refreshed.Please confirm this schedule change aligns with your data refresh requirements and timezone considerations (America/Los_Angeles).
card_data/pipelines/defs/load/tcgdex/load_sets.py (1)
1-14: LGTM! Import structure and SODA_PATH resolution are appropriate.The fallback pattern for
SODA_PATHensures the tool works across different environments..github/workflows/ci.yml (4)
34-34: LGTM! Version bump to v1.8.3.The version number update is consistent with the PR title and objectives.
62-78: LGTM! Gitleaks security scanning integration.Adding gitleaks as a security gate before build steps is a good security practice. The configuration with
fetch-depth: 0and explicit version pinning (8.29.0) is appropriate.
50-50: actions/checkout@v6 is a valid, stable release. No action required. The version upgrade from v4 to v6 is official and stable, with documented compatibility notes for Docker containers (requires Actions Runner v2.329.0+).
442-442: The upgrade togithub/codeql-action/upload-sarif@v4is valid. Version v4 was released on October 7, 2025, and is compatible. GitHub has announced the deprecation of v3 with support ending in December 2026, making v4 the current recommended version.card_data/pipelines/defs/load/tcgdex/load_series.py (2)
1-14: LGTM! Import structure and SODA_PATH resolution.The setup matches the pattern across other load modules and is well-structured.
36-65: LGTM! Data quality check implementation is solid.The Soda integration with proper error handling and output capture works well.
card_data/pipelines/defs/extract/tcgdex/extract_sets.py (3)
1-19: LGTM! Model definition is appropriate.The
Setmodel with optional fields for card counts and nullable logo/symbol is well-designed for the TCGDex API response structure.
21-46: LGTM! Data extraction and flattening logic is clean.The URL iteration and nested data extraction with proper field mapping is well-implemented.
48-56: LGTM! Validation pattern is consistent.The Pydantic validation with colored output matches the pattern used in
extract_series.pyand provides good error feedback.docs/Infrastructure_Guide/local-deployment.md (2)
6-11: LGTM! Documentation restructuring improves clarity.The updated introduction clearly explains the tool progression (Dagster → dbt → Soda) and their dependencies, which helps users understand the setup order.
186-281: LGTM! Comprehensive Soda documentation.The new Soda section provides excellent guidance with:
- Clear installation instructions
- Configuration file examples
- Environment variable setup
- Scan execution steps
- Expected output samples
This significantly improves the local deployment experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/coverage.yml (1)
31-31: Consider the implications of allowing codecov failures.Setting
fail_ci_if_error: falsemeans the CI pipeline will continue even if coverage upload fails. While this prevents Codecov service outages from blocking PRs, it also means genuine upload failures (e.g., authentication issues, network problems) won't be noticed until someone manually checks coverage reports.If this is intentional to improve developer experience, consider adding a comment explaining the rationale.
📝 Optional: Add explanatory comment
- name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: + # Allow CI to pass even if Codecov upload fails to prevent service outages from blocking PRs fail_ci_if_error: false token: ${{ secrets.CODECOV_ORG_TOKEN }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/coverage.yml
🔇 Additional comments (1)
.github/workflows/coverage.yml (1)
14-14: actions/checkout@v6 upgrade is safe.v6 is released and stable. While it has breaking changes (credentials handling in git config, Docker actions now require runner v2.329.0+), they don't affect this workflow since it runs on a standard ubuntu-22.04 runner with no Docker-based git operations or credential-dependent git commands.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
card_data/pipelines/utils/json_retriever.py
🔇 Additional comments (1)
card_data/pipelines/utils/json_retriever.py (1)
1-1: No action required. Version 2.32.4 of therequestslibrary is secure and free from known public CVEs; it actually contains the fix for CVE-2024-47081. The codebase already specifiesrequests>=2.32.4incard_data/pyproject.toml, which ensures the patched version is used.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/berry/berry_test.go (1)
67-116: Fix the test logic - key messages are not being tested.The test has critical logic errors:
- Line 107 calls
m.Update(nil)with a nil message, but the test cases define differentkeyMsgvalues that are never used.- The test doesn't actually send key messages to validate different key handling behaviors.
- Lines 109-113 check if the model is nil when
shouldQuitis true, which doesn't properly validate quitting behavior.The test provides false confidence as it passes without actually testing the intended key handling logic.
🔎 Proposed fix for the test logic
func TestModelUpdate(t *testing.T) { // Create a simple table for testing columns := []table.Column{{Title: "Berry", Width: 16}} rows := []table.Row{{"TestBerry"}} testTable := table.New( table.WithColumns(columns), table.WithRows(rows), table.WithFocused(true), table.WithHeight(5), ) - m := model{ - table: testTable, - } - tests := []struct { name string keyMsg string shouldQuit bool expectError bool }{ { name: "escape key", keyMsg: "esc", shouldQuit: true, }, { name: "ctrl+c key", keyMsg: "ctrl+c", shouldQuit: true, }, { name: "other key", keyMsg: "j", shouldQuit: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - updated, _ := m.Update(nil) + m := model{table: testTable} + var msg tea.Msg + + switch tt.keyMsg { + case "esc": + msg = tea.KeyMsg{Type: tea.KeyEsc} + case "ctrl+c": + msg = tea.KeyMsg{Type: tea.KeyCtrlC} + case "j": + msg = tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'j'}} + } + + updated, _ := m.Update(msg) + updatedModel := updated.(model) if tt.shouldQuit { - if updated == nil { - t.Errorf("Update() returned nil model") + if !updatedModel.quitting { + t.Errorf("Expected model to be quitting, but quitting is false") + } + } else { + if updatedModel.quitting { + t.Errorf("Expected model to not be quitting, but quitting is true") } } }) } }
🧹 Nitpick comments (4)
docs/Infrastructure_Guide/local-deployment.md (2)
51-63: Specify a language for the directory-structure code block.The markdown linter expects a language identifier for fenced code blocks. For ASCII directory trees, use
plaintextortreeas the language hint.🔎 Proposed fix
-``` +```plaintext . └── pipelines/ └── defs/ ├── extract/ │ ├── extract_data.py │ └── extract_pricing_data.py ├── load/ │ ├── load_data.py │ └── load_pricing_data.py └── transform/ └── transform_data.py -``` +```
209-209: Clarify that thecard_data/pipelines/soda/directory must be created manually.The documentation instructs users to create
configuration.ymlandchecks.ymlfiles undercard_data/pipelines/soda/, but doesn't explicitly mention that this directory structure must be created first. Add a step to create the directory before proceeding.🔎 Proposed guidance
Add this step before line 209:
Create the directory structure for Soda configuration and checks: \`\`\`shell mkdir -p card_data/pipelines/soda \`\`\`docs/Infrastructure_Guide/index.md (1)
110-117: Consider reducing the number of exclamation marks for a more measured tone.The note block contains multiple exclamation marks (6 total) across 3 sentences, which may feel overly enthusiastic. Consider replacing one or two with periods for a more balanced professional tone.
🔎 Example adjustment
!!! note - This project is a learning playground for exploring new tools, services, and programming languages. - Some design choices are intentionally experimental or may not follow conventional patterns— - that's part of the learning process! + This project is a learning playground for exploring new tools, services, and programming languages. + Some design choices are intentionally experimental or may not follow conventional patterns— + that's part of the learning process. - Feedback and suggestions are always welcome! If you spot an issue or have ideas for improvement, + Feedback and suggestions are always welcome. If you spot an issue or have ideas for improvement, please open a [GitHub Issue](https://github.com/digitalghost-dev/poke-cli/issues).cmd/search/search_test.go (1)
139-160: Good View() test coverage with one suggestion.The three new tests improve coverage for different Model view states. However,
TestModelViewShowResultshas a weak assertion.Suggestion: In
TestModelViewShowResults, consider strengthening the assertion to verify that the actualSearchResultscontent ("Test Results") appears in the rendered view:func TestModelViewShowResults(t *testing.T) { m := Model{ ShowResults: true, SearchResults: "Test Results", } view := m.View() - // View calls RenderInput when ShowResults is true - assert.NotEmpty(t, view, "View should render results") + assert.Contains(t, view, "Test Results", "View should render the search results") }This ensures the test validates that the results are actually rendered, not just that the view produces non-empty output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
card_data/pipelines/defs/load/tcgdex/load_sets.pycard_data/pipelines/defs/transform/transform_data.pycli_test.gocmd/berry/berry_test.gocmd/search/search_test.gocmd/types/types_test.godocs/Infrastructure_Guide/index.mddocs/Infrastructure_Guide/local-deployment.md
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/types/types_test.go
- card_data/pipelines/defs/transform/transform_data.py
🧰 Additional context used
🧬 Code graph analysis (2)
card_data/pipelines/defs/load/tcgdex/load_sets.py (2)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_secret(13-23)card_data/pipelines/defs/extract/tcgdex/extract_sets.py (1)
extract_sets_data(22-56)
cmd/search/search_test.go (1)
cmd/search/search.go (2)
SearchCommand(14-47)Model(50-58)
🪛 LanguageTool
docs/Infrastructure_Guide/index.md
[style] ~109-~109: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1514 characters long)
Context: ...├── pyproject.toml └── uv.lock ``` !!! note This project is a learning pl...
(EN_EXCESSIVE_EXCLAMATION)
docs/Infrastructure_Guide/local-deployment.md
[style] ~187-~187: Using many exclamation marks might seem excessive (in this case: 12 exclamation marks for a text that’s 4658 characters long)
Context: ...om the staging schema. --- ## Soda !!! question "What is Soda?" Soda is a...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/Infrastructure_Guide/index.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
docs/Infrastructure_Guide/local-deployment.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
71-71: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
119-119: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
190-190: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
225-225: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🪛 Ruff (0.14.10)
card_data/pipelines/defs/load/tcgdex/load_sets.py
41-41: subprocess call: check for execution of untrusted input
(S603)
63-65: Create your own exception
(TRY002)
63-65: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (15)
cmd/search/search_test.go (2)
90-95: LGTM! Error handling properly updated.The test correctly adapts to the new
SearchCommand()signature that returns an error instead of callingos.Exit(). The error assertions align with the test case expectations.
128-137: Excellent addition for validation error coverage.This test properly verifies that
SearchCommand()returns an error when invalid arguments are provided, improving test coverage for the error handling path.cli_test.go (2)
8-11: LGTM! Import reorganization improves structure.The imports are properly organized and all are used within the test file. The addition of
testify/assertenables cleaner assertions inTestRunCLI.
140-140: Good addition! The test correctly validates error handling for invalid search arguments.The search command implementation properly validates arguments and rejects cases with too many arguments. The test case aligns with the validation logic in
cmd/utils/validateargs.go, which limits search arguments to 3 (command name only; flags handled separately). This improves integration-level test coverage for error paths.cmd/berry/berry_test.go (7)
1-15: LGTM! Well-organized imports for comprehensive UI testing.The imports appropriately cover all necessary dependencies for Bubble Tea UI testing, including teatest for terminal interaction simulation and testify for assertions.
17-57: LGTM! Proper table-driven test with cleanup.The test correctly validates help flag behavior and properly manages os.Args manipulation with defer-based cleanup.
59-65: LGTM!Simple and correct initialization test.
118-136: LGTM!The test correctly validates view rendering for both normal and quitting states.
138-167: LGTM!The test properly validates that key UI elements are present in the rendered view.
169-197: LGTM! Excellent test helper for database-free testing.The
createTestModelfunction is well-structured and properly configures a table with test data and styling, enabling UI interaction tests without database dependencies.
199-255: LGTM! Comprehensive teatest-based UI interaction tests.The navigation, quit, and initial selection tests properly use teatest to simulate terminal interactions and validate final model state. The tests cover key user interaction scenarios effectively.
card_data/pipelines/defs/load/tcgdex/load_sets.py (4)
1-13: LGTM! Clean imports and sensible SODA_PATH configuration.The imports are well-organized, and the SODA_PATH fallback logic properly handles cases where
sodamight not be in the system PATH.
16-33: Well-structured loading asset with appropriate resilience.The retry policy with exponential backoff is well-suited for handling transient database connection issues. The
if_table_exists="replace"strategy is acceptable for a staging table.
36-37: Previous naming issue has been resolved.The function is now correctly named
data_quality_check_on_sets()matching its purpose (checking sets data) and the asset name"data_quality_checks_on_sets". This resolves the issue flagged in the previous review.
41-54: Path resolution verified as correct and consistent.The relative paths
"../../../soda/configuration.yml"and"../../../soda/checks_sets.yml"correctly resolve tocard_data/pipelines/soda/from the module's location. Both configuration files exist, and this pattern matches identically withload_series.py(which uses the same relative paths to access the same soda directory). Thecwdparameter ensures relative paths work correctly from the module's parent directory.
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/Infrastructure_Guide/index.md (1)
110-117: Minor style note: consider reducing exclamation marks.The note block contains 6 exclamation marks across 1,514 characters, which static analysis flags as excessive. While not a functional issue, consider rewording one or two sentences to use periods instead—e.g., changing the final sentence to "Feedback and suggestions are always welcome. If you spot an issue or have ideas for improvement, please open a GitHub Issue." This is a minor polish and entirely optional.
.github/workflows/coverage.yml (1)
31-31: Consider whether silently ignoring Codecov failures is desired.Setting
fail_ci_if_error: falsemeans the CI will pass even if the Codecov upload fails. This could mask issues such as authentication problems, broken integration, or coverage report generation errors.If this is intentional (e.g., to prevent flaky Codecov infrastructure from blocking PRs), consider documenting the rationale. Otherwise, you may want to keep the default behavior of failing on errors.
cmd/berry/berry_test.go (1)
199-224: LGTM! Thorough navigation test with correct assertions.The navigation logic correctly verifies that down→down→up from index 0 lands on index 1 ("Aspear").
Optional: Consider increasing timeout for CI stability.
The 300ms timeout at line 212 (and similar timeouts in other UI tests) may be tight in slower CI environments. Consider increasing to 500ms or 1s if flakiness occurs.
🔎 Optional timeout adjustment
- testModel.WaitFinished(t, teatest.WithFinalTimeout(300*time.Millisecond)) + testModel.WaitFinished(t, teatest.WithFinalTimeout(time.Second))card_data/pipelines/tests/extract_series_test.py (1)
37-41: Remove unnecessary# noseccomments from assertions.The
# noseccomments on these assertions appear unnecessary. Thenosecdirective is typically used to suppress security linter warnings (e.g., from Bandit) for operations that might be flagged as risky, such as subprocess calls or SQL queries. Standard test assertions should not trigger security warnings.🔎 Suggested cleanup
- assert isinstance(result, pl.DataFrame) # nosec - assert len(result) == 3 # nosec - assert set(result["id"].to_list()) == {"swsh", "sv", "me"} # nosec - assert "name" in result.columns # nosec - assert "logo" in result.columns # nosec + assert isinstance(result, pl.DataFrame) + assert len(result) == 3 + assert set(result["id"].to_list()) == {"swsh", "sv", "me"} + assert "name" in result.columns + assert "logo" in result.columnscard_data/pipelines/defs/load/tcgdex/load_series.py (4)
13-13: Improve SODA_PATH fallback handling.The fallback
or "soda"will still fail if thesodaexecutable isn't in the system PATH. Consider failing early with a clear error message or validating the executable exists during module initialization.🔎 Proposed improvement
-SODA_PATH = shutil.which("soda") or "soda" +SODA_PATH = shutil.which("soda") +if SODA_PATH is None: + raise RuntimeError( + "Soda executable not found in PATH. Please install Soda or ensure it's accessible." + )
16-33: LGTM with minor style suggestion.The asset is well-configured with retry policy and appropriate error handling for staging data loads. The
if_table_exists="replace"strategy is acceptable for staging tables.Minor style improvement:
🔎 Remove redundant assignment
- df = extract_series_data try: - df.write_database( + extract_series_data.write_database( table_name=table_name, connection=database_url, if_table_exists="replace" )
41-54: Subprocess call is secure but could benefit from file validation.The subprocess call is properly secured with hardcoded arguments (no user input), making the
# nosec B603comment appropriate. However, consider validating that the Soda configuration files exist before executing the subprocess to provide clearer error messages.🔎 Optional: Add configuration file validation
def data_quality_check_on_series() -> None: current_file_dir = Path(__file__).parent print(f"Setting cwd to: {current_file_dir}") + + # Validate configuration files exist + config_path = current_file_dir / "../../../soda/configuration.yml" + checks_path = current_file_dir / "../../../soda/checks_series.yml" + if not config_path.resolve().exists(): + raise FileNotFoundError(f"Soda configuration not found: {config_path}") + if not checks_path.resolve().exists(): + raise FileNotFoundError(f"Soda checks file not found: {checks_path}") result = subprocess.run( # nosec B603
62-65: Consider a custom exception class (optional).The static analysis tools suggest creating a custom exception class instead of using the generic
Exception. While the current implementation is functional for Dagster's error handling, a custom exception would improve code clarity and allow for more specific error handling upstream.🔎 Optional: Custom exception class
At the module level:
class SodaCheckFailedError(Exception): """Raised when Soda data quality checks fail.""" def __init__(self, return_code: int): self.return_code = return_code super().__init__(f"Soda data quality checks failed with return code {return_code}")Then update the check:
if result.returncode != 0: - raise Exception( - f"Soda data quality checks failed with return code {result.returncode}" - ) + raise SodaCheckFailedError(result.returncode)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
card_data/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/codspeed.yml.github/workflows/coverage.yml.github/workflows/go_lint.yml.github/workflows/go_test.yml.github/workflows/release.yml.gitignorecard_data/pipelines/defs/load/tcgdex/load_series.pycard_data/pipelines/tests/extract_series_test.pycard_data/pyproject.tomlcmd/berry/berry_test.godocs/Infrastructure_Guide/index.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/release.yml
- .github/workflows/go_test.yml
- card_data/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/berry/berry_test.go (2)
styling/styling.go (1)
Color(83-85)cmd/berry/berry.go (1)
BerryCommand(18-53)
card_data/pipelines/defs/load/tcgdex/load_series.py (2)
card_data/pipelines/utils/secret_retriever.py (1)
fetch_secret(13-23)card_data/pipelines/defs/extract/tcgdex/extract_series.py (1)
extract_series_data(18-36)
card_data/pipelines/tests/extract_series_test.py (1)
card_data/pipelines/defs/extract/tcgdex/extract_series.py (1)
extract_series_data(18-36)
🪛 LanguageTool
docs/Infrastructure_Guide/index.md
[style] ~109-~109: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1514 characters long)
Context: ...├── pyproject.toml └── uv.lock ``` !!! note This project is a learning pl...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/Infrastructure_Guide/index.md
53-53: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 Ruff (0.14.10)
card_data/pipelines/defs/load/tcgdex/load_series.py
41-41: subprocess call: check for execution of untrusted input
(S603)
63-65: Create your own exception
(TRY002)
63-65: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (13)
docs/Infrastructure_Guide/index.md (2)
51-108: Code-block style conflict: verify fenced vs. indented format.The code block at line 53 now has the
textlanguage identifier (addressing the previous review), but markdownlint flags MD046—it expects an indented code block rather than a fenced one. This creates a conflict with the earlier requirement.Options:
- Keep the fenced block with
textidentifier if language specification takes precedence in your linter config.- Switch to an indented code block (4+ spaces) if markdownlint rules take precedence.
Please verify which convention your project enforces and adjust accordingly.
51-108: Excellent structural improvements.The new "Project Layout" section clearly maps the directory hierarchy and shows the restructured extract/load modules (tcgcsv, tcgdex) and newly integrated Soda checks. The directory tree is comprehensive, properly formatted, and the typo from the previous review (
utls/→utils/) is correctly fixed. The inclusion ofutils/,soda/, and the Soda configuration files aligns well with the PR objectives around restructuring card data handling and adding Soda validation..github/workflows/go_lint.yml (1)
15-23: LGTM! CI dependency updates look good.The updates to Go 1.24 (released February 2025) and actions/checkout@v6 (released November 2025) are both valid and stable. The runner pinning to ubuntu-22.04 provides better reproducibility than ubuntu-latest.
.github/workflows/coverage.yml (1)
14-19: LGTM! CI dependency updates are valid.Go 1.24 (released February 2025) and actions/checkout@v6 (released November 2025) are both stable releases.
cmd/berry/berry_test.go (5)
7-7: LGTM! Well-organized test dependencies.The new imports appropriately support the UI testing scaffolding with Bubble Tea, styling, and assertion helpers.
Also applies to: 10-14
169-197: LGTM! Clean test fixture helper.The
createTestModel()function provides a well-structured in-memory model with consistent test data and styling, avoiding database dependencies.
226-239: LGTM! Clean escape key quit verification.The test correctly verifies that pressing Escape sets the quitting state.
241-255: LGTM! Good coverage of initial state.The test correctly verifies that the first row ("Aguav") is selected by default before any navigation occurs.
257-267: LGTM! Validation error test is correct, and the previous copy-paste issue has been resolved.The test properly exercises error handling when extra arguments are provided. Line 265 now correctly references "BerryCommand" in the error message, addressing the previous review comment.
.gitignore (1)
69-70: LGTM!The additions appropriately ignore developer-specific documentation and CodSpeed benchmark artifacts generated by the new workflow.
.github/workflows/codspeed.yml (2)
1-32: LGTM!The workflow setup is well-configured with appropriate triggers, Python version, and dependency management using uv.
8-10: The workflow configuration is correct. CodSpeed documentation explicitly requires theid-token: writepermission even when usingmode: simulationfor OIDC-based authentication. The current permissions setup matches the recommended CodSpeed configuration and should not be modified.Likely an incorrect or invalid review comment.
card_data/pipelines/tests/extract_series_test.py (1)
9-9: LGTM!The import path update correctly references the refactored tcgdex module structure, and the benchmark marker appropriately integrates with the new CodSpeed workflow.
Also applies to: 22-22
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.