fix(parser): resolve model bases across the whole scan, not per-file (#20) - #22
Merged
Merged
Conversation
The 2-pass transitive resolution in parse_models_file only matched a model's base classes against definitions collected from the SAME parsed file. A concrete model inheriting an abstract base defined in another scanned module (e.g. a shared common/models.py) was silently dropped from scan_workspace's output, since the base's name never entered is_model_name. Split parse_models_file into _collect_defs (pass 1: gather class defs) and _resolve_and_filter (pass 2: transitive resolution + abstract drop). parse_models_file composes both, unchanged for single-file callers. scan_workspace now collects defs from every file first and calls _resolve_and_filter once over the union, so bases defined in a different scanned file are found. Adds a two-module regression fixture (cross_module/common + app) and test proving Article(TimeStampedModel) is no longer dropped when the base lives in a different file. Fixes FROWNINGdev#20. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TimeStampedModel (the name used in issue FROWNINGdev#20's illustrative example) is one of the hardcoded heuristic tail names in _looks_like_model, so a class inheriting it is recognized as a model by name alone, without any base resolution — the fixture test passed even against the unfixed per-file parser, silently failing to exercise the bug. Renamed to TimeStampBase, which does not match any heuristic pattern; the fixture now only passes when the base is genuinely resolved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #20.
What
scan_workspacenow collects class definitions from every parsed file first, then runs the transitiveis_model_nameresolution once over the union — so a base class only needs to be defined somewhere in the scanned tree, not in the same file as its subclass. Mechanically,parse_models_file's tail was split into_collect_defs(pass 1, per file) and_resolve_and_filter(pass 2);parse_models_filecomposes both, so single-file behavior (and the parity/abstract tests that call it directly) is unchanged, andscan_workspacecalls pass 2 once over all files' defs, regrouping resolved models back into their apps by file path afterward.Includes the two-module regression fixture from the issue:
common/models.pydefines an abstractTimeStampBase,app/models.pydefinesArticle(TimeStampBase), and the test assertsArticlesurvives the scan (it returns[]on the current parser).A gotcha worth knowing about
The issue's illustrative name
TimeStampedModelis a trap: it is hardcoded in_looks_like_model's heuristic pattern list, so a fixture using that literal name is recognized by name alone and the regression test passes even against the unfixed parser. Caught by mutation-testing the test itself — the fixture base is deliberately namedTimeStampBase, with a comment explaining why.Honest notes
BlogCategorycase does not newly appear:mezzanine/core/models.py(whereSluggedlives) was never vendored byscripts/fetch_golden_fixtures.py, and no resolution scope can find a definition that isn't in the tree. Happy to extend the fetch script to vendor it as a follow-up if you want the golden fixture to demonstrate this fix.tests/fixtures/golden/directory at once (as the timing test does) picks up django-cms'sPage/PageTypebecause wagtail's unrelatedPageis a real model with the same name. In single-project use this only bites if two genuinely different classes in one project share a class name. No test assertions are sensitive to it; flagging it so the choice is yours.cli/.Verification
parser.pyto main fails exactly 1 test —test_model_with_cross_module_base_is_present,AssertionError: 'Article' not found in []— collection stays at 50, so the test genuinely bites.Generated by Claude Fable 5 (brief, review), Claude Sonnet 5 (implementation)
Summary by CodeRabbit
Bug Fixes
Tests