Add test for consume name to name mapping #867
Conversation
| continue | ||
|
|
||
| if key in spec_mapping: | ||
| mapping[value] = Field(name=value, type=mapping.pop(key).type) | ||
| if operations_column_name in operations_schema: |
There was a problem hiding this comment.
We are able to add a check for addtionalProperties here, and ignore this exception if additionalProperties are allowed. However, in that case we have to change the string mapping to a string: pa.DataType mapping to avoid issues later on. The ComponentOp would need to access the dataset schema.
I think it might be cleaner to implement this behaviour before we are initialising the ComponentOp, infer the consumes, validate and fix it (if needed). The ComponentOp will be initialised with a valid consumes definition afterwards.
There was a problem hiding this comment.
Agree, see my comment below.
There was a problem hiding this comment.
Thanks @mrchtr! I think we can indeed refactor a bit more to split the functionality more cleanly.
| @@ -145,13 +146,18 @@ def __init__( | |||
| cache: t.Optional[bool] = True, | |||
| resources: t.Optional[Resources] = None, | |||
| component_dir: t.Optional[Path] = None, | |||
| dataset_fields: t.Optional[t.Mapping[str, Field]] = None, | |||
There was a problem hiding this comment.
I'm wondering if we shouldn't split all this consumes / produces logic into a separate class, eg. a ComponentOpFactory. I think it could help with maintainability and testability to keep the ComponentOp limited to a representation of the operation, close to a dataclass.
There was a problem hiding this comment.
I think it makes sense. I'll create an issue for it that we can tackle separately.
There was a problem hiding this comment.
Ok, but would prefer to do it as a follow-up right away. I think this change tips it to the point where this is needed, since we're starting to mingle the operation and dataset.
| continue | ||
|
|
||
| if key in spec_mapping: | ||
| mapping[value] = Field(name=value, type=mapping.pop(key).type) | ||
| if operations_column_name in operations_schema: |
There was a problem hiding this comment.
Agree, see my comment below.
mrchtr
changed the base branch from
feature/renaming-inner-outer-consumes-produces
to
main
There was a problem hiding this comment.
Thanks @mrchtr.
Looks good, but let's make sure all the naming is consistent. I left some suggestions.
| self._operation_consumes: t.Optional[t.Mapping[str, Field]] = None | ||
| self._consumes_from_dataset: t.Optional[t.Mapping[str, Field]] = None | ||
| self._operation_produces: t.Optional[t.Mapping[str, Field]] = None | ||
| self._produces_to_dataset: t.Optional[t.Mapping[str, Field]] = None |
| @@ -145,13 +146,18 @@ def __init__( | |||
| cache: t.Optional[bool] = True, | |||
| resources: t.Optional[Resources] = None, | |||
| component_dir: t.Optional[Path] = None, | |||
| dataset_fields: t.Optional[t.Mapping[str, Field]] = None, | |||
There was a problem hiding this comment.
Ok, but would prefer to do it as a follow-up right away. I think this change tips it to the point where this is needed, since we're starting to mingle the operation and dataset.
Fix #863