fix: selector use provided models and fix mypy#5148
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two issues: a mypy error where models could be None and a bug where the provided models parameter wasn't being used in the Git selector logic. The changes rename the models variable to all_models to avoid confusion and ensure all references use the resolved models dictionary.
- Resolved mypy error by properly handling the optional
modelsparameter - Fixed bug where Git selector used
self._modelsinstead of the providedmodelsparameter - Improved code clarity by renaming variables to avoid shadowing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
sqlmesh/core/selector.py
Outdated
| model | ||
| for tag, models in models_by_tags.items() | ||
| for model in models | ||
| for model in all_models |
There was a problem hiding this comment.
This line should iterate over models (the model names from the tag mapping) rather than all_models. The correct iteration should be for model in models to match the models found for the specific tag pattern.
| for model in all_models | |
| for model in models |
There was a problem hiding this comment.
I don't see why this would be the case.
There was a problem hiding this comment.
Ah actually this was correct. models is redefined in the for loop. 🤦
eakmanrq
force-pushed
the
eakmanrq/selector_bug_and_mypy
branch
from
e67079b to
5766ad2
Compare
|
do we need any tests here? |
Was getting a mypy issue since it would complain that
modelscould be None. Reviewing and I identified a bug on line 216 where we didn't use the provided models. Therefore address both in this PR.