feat: add data config checker for property value types #2328
Conversation
| if property_types := data_node_config._PROPERTIES_TYPES.get(data_node_config.storage_type): | ||
| for prop_key, prop_type in property_types.items(): | ||
| prop_value = data_node_config.properties.get(prop_key) if data_node_config.properties else None | ||
| if prop_value and not isinstance(prop_value, prop_type): |
There was a problem hiding this comment.
prop_type is the value in the _PROPERTIES_TYPES dictionary. Today, it must be a Python type.
Can we imagine prop_type being either a type or a list of types? Then, we should check that the type of prop_value is among the types in the prop_type list of types.
Does it make sense, or is it just over-engineering?
There was a problem hiding this comment.
isinstance can take a tuple of type as the second parameter. So we only need to define the proprety as a tuple of expected types and it should be good to go. I had to do that for a couple of properties already.
jrobinAV
added
Core
joaoandre-avaiga
force-pushed
the
feature/#2298-add-value-config-checker
branch
from
c82433d to
70c47f1
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be" | ||
| f" populated with a serializable Callable function but not a lambda.", | ||
| ) | ||
| def _check_property_types(self, data_node_config_id: str, data_node_config: DataNodeConfig): |
There was a problem hiding this comment.
can you help propagate this change on enterprise as well? I'm quite certain it will fail on enterprise :D thanks!
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
How to reproduce the issue
Please replace this line with instructions on how to reproduce the issue or test the feature.
Other branches or releases that this needs to be backported
Describe which projects this change will impact and that needs to be backported.
Checklist
We encourage you to keep the code coverage percentage at 80% and above.