Case sensitivity #193
Case sensitivity #193
Conversation
There was a problem hiding this comment.
@dave-connors-3 I've left a few comments. One around cleaning up a print statement, and a couple of recommendations around nitpicky code-smell thoughts. Removing the print statement is the only blocking change in this review.
dbt_meshify/utilities/contractor.py
Outdated
| @@ -15,11 +15,18 @@ def generate_contract(self, model: ModelNode) -> ResourceChange: | |||
| """Generate a ChangeSet that adds a contract to a Model.""" | |||
| model_catalog = self.project.get_catalog_entry(model.unique_id) | |||
|
|
|||
| # create a mapping of the column name to its representation in the yml file to maintain the original case | |||
| original_case = {column_name.lower(): column_name for column_name in model.columns.keys()} | |||
| print(original_case) | |||
There was a problem hiding this comment.
Might want to clean this print statement up.
There was a problem hiding this comment.
lol i caught this earlier and never pushed smdh
| if not model_catalog or not model_catalog.columns: | ||
| columns = None | ||
| else: | ||
| columns = [ | ||
| {"name": name.lower(), "data_type": value.type.lower()} | ||
| { | ||
| "name": original_case.get(name.lower()) or name.lower(), |
There was a problem hiding this comment.
This is totally correct and likely ideal due to readability! Another way of doing this is through the use of get's second argument.
original_case.get(name.lower(), name.lower())| @pytest.fixture | ||
| def change(project, model) -> ResourceChange: | ||
| contractor = Contractor(project=project) | ||
| return contractor.generate_contract(model) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def all_cap_model_change(project, all_cap_model) -> ResourceChange: | ||
| contractor = Contractor(project=project) | ||
| return contractor.generate_contract(all_cap_model) |
There was a problem hiding this comment.
This isn't terrible, but I see what you mean by code smell. I suspect that this fixture could follow a factory pattern instead, in which the fixture returns a function that itself generates or returns models that have different capitalization configurations.
There was a problem hiding this comment.
@dave-connors-3 these changes are both necessary and sufficient. ✅
closes #190
As reported, when creating a contract, all column names are currently standardized to lowercase, even if the column was in uppercase in the original yml, leading to unintended duplications/overwriting. This PR adds a step to the Contractor class to maintain the original casing of each of the columns from the yml entry. Anything not defined in yml, but present in the catalog will continue to be lowercased.
I could not figure out a clean way to pass dynamic column content to our pytest fixture in the unit tests, so i created a new model fixture that included the column specifications. I think that's a big code smelly, so open to suggestions.