fix(genbi): tolerate non-list columns in model inventory - #2614
Conversation
columns is YAML-sourced; only validate the list container. Models still arrive from load_models (already dict-filtered).
WalkthroughGenBI model inventory formatting now tolerates non-list columns and mixed column entry types. A unit test verifies that build instruction composition preserves model names and valid column content for malformed inventory inputs. ChangesModel inventory formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_genbi_build.py (1)
101-116: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the filtering behavior, not only that formatting does not crash.
The test does not verify that
"bare"is preserved or that numeric3is ignored; the existingidassertion is satisfied by the dictionary entry alone. Assert the rendered inventory lines for both models.Based on the PR objective, the test should cover non-list-to-empty normalization and mixed-entry filtering.
Proposed assertion update
- assert "**orders**" in text - assert "**customers**" in text - assert "id" in text + assert "- **orders**: \n" in text + assert "- **customers**: id, bare" in text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/tests/unit/test_genbi_build.py` around lines 101 - 116, Strengthen the assertions in the compose_build_instruction test to verify rendered inventory lines for both orders and customers, including preservation of the bare string entry and exclusion of numeric 3. Ensure the assertions cover non-list columns normalization to an empty list while still validating the expected id and bare entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/wren/tests/unit/test_genbi_build.py`:
- Around line 101-116: Strengthen the assertions in the
compose_build_instruction test to verify rendered inventory lines for both
orders and customers, including preservation of the bare string entry and
exclusion of numeric 3. Ensure the assertions cover non-list columns
normalization to an empty list while still validating the expected id and bare
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf1cb698-0f8d-4393-a595-e04b7482c0a6
📒 Files selected for processing (2)
core/wren/src/wren/genbi/composer.pycore/wren/tests/unit/test_genbi_build.py
Summary
_format_model_inventorytreats a non-listcolumnsvalue from YAML as empty instead of crashing on join.Motivation
Closed #2548 put a dead model-level guard in the formatter. Maintainer: only the
columnscontainer type is reachable; models already come fromload_models.Test plan
test_build_inventory_tolerates_non_list_columnsviacompose_build_instructionSummary by CodeRabbit