Dhole clean up #91
Dhole clean up #91
Conversation
Pull Request Test Coverage Report for Build 13269764680Details
💛 - Coveralls |
| target_schema.setdefault("properties", {})[property_name] = deepcopy(source_schema) | ||
|
|
||
|
|
||
| def resolve_schema_object_path(json_schema: Mapping[str, Any], path: str) -> Any: |
There was a problem hiding this comment.
I am not really sure about the place of this function. In terms of its purpose, it partially fits in model_transform but at the same time, it is more of a helper function to modify the json schema that a path leads to. Also, grouping this with resolve_data_object_path seems more natural to me. How about we create path_utils.py and add them there?
Or maybe a better option would be creating a resolve_path module that encapsulates resolve_path.py which can be renamed as resolve_path_relations.py and maybe something like resolve_path_properties.py that includes the functions resolve_schema_object_path and resolve_data_object_path?
| @@ -175,3 +172,25 @@ def get_modification_target( | |||
| if not isinstance(target, dict) or property in target: | |||
| raise EvitableTransformationError() | |||
| return target | |||
|
|
|||
|
|
|||
| def resolve_data_object_path(data: Mapping, path: str) -> Any: | |||
There was a problem hiding this comment.
I have the same suggestion/comment I made to resolve_schema_object_path
| from metldata.builtin_transformations.common import SourcePath | ||
|
|
||
|
|
||
| class CopyContentSchemaPath(BaseSettings): |
There was a problem hiding this comment.
I think the reason we wanted to separate CopyContentSchema instead of using the NewContentSchemaPath was to reduce the config file fields for the empty object_path. But if this will cause typing problems requiring ignore, I think going back to how it was is preferable here, i.e. replacing CopyContentSchema with NewContentSchemaPath.
| @@ -56,3 +56,13 @@ class TargetSourceInstructionProtocol(Protocol): | |||
| Instruction = TypeVar( | |||
| "Instruction", bound=TargetInstructionProtocol | SourceInstructionProtocol | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Adding InstructionProtocol to Instruction bound solves the type problem.
| Instruction = TypeVar( | |
| "Instruction", bound=TargetInstructionProtocol | SourceInstructionProtocol | InstructionProtocol | |
| ) |
There was a problem hiding this comment.
Could you elaborate on this one?
Once I remove the type: ignores it's mypy is back to complaining
| @@ -39,4 +39,4 @@ def check_model_assumptions( | |||
| assert_class_is_source(path=path, instruction=instruction) | |||
| assert_path_classes_and_relations_exist(model=schema, path=path) | |||
| assert_no_relation_target_multiplicity(model=schema, path=path) | |||
| assert_object_path_exists(model=schema, instruction=instruction) | |||
| assert_object_path_exists(model=schema, instruction=instruction) # type: ignore | |||
There was a problem hiding this comment.
Please see my comment below about using NewContentSchemaPath in copy_content.
| @@ -32,7 +32,7 @@ def copy_content( | |||
| """Apply all transformation instructions.""" | |||
| return transform_data( | |||
| data=data, | |||
| instructions_by_class=instructions_by_class, | |||
| instructions_by_class=instructions_by_class, # type: ignore | |||
There was a problem hiding this comment.
Please see my comment above related to the Instruction type. That should solve the type problem we suppress here.
| @@ -62,7 +60,7 @@ def add_content_schema_copy( | |||
| raise EvitableTransformationError() from exc | |||
|
|
|||
| add_property_per_instruction( | |||
| cls_instruction=instruction, | |||
| cls_instruction=instruction, # type: ignore | |||
There was a problem hiding this comment.
Going back to NewContentSchemaPathshould also resolve this.
| @@ -9,14 +9,12 @@ copy_content: | |||
| - class_name: Sample | |||
| target_content: | |||
| property_name: referenced_filename | |||
| object_path: "" | |||
There was a problem hiding this comment.
I think going back to this version is better.
No description provided.