Feature/#521 refactor code for databricks integration #2284
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| def _check_callable(self, data_node_config_id: str, data_node_config: DataNodeConfig): | ||
| properties_to_check = { | ||
| @staticmethod | ||
| def _get_class_type_and_properties() -> Dict[str, List[Tuple[Any, List[str]]]]: |
There was a problem hiding this comment.
this can be turned into a class attribute instead of a function, as we were using function, I just went with it so can quickly implement it
There was a problem hiding this comment.
I agree. We can implement it as a dictionary class attribute of the DataNoceConfig class. Just like we have _REQUIRED_PROPERTIES: Dict[str, List] or _OPTIONAL_PROPERTIES: Dict[str, Dict[str, Any]], we could have something like :
_PROPERTIES_TYPES: Dict[str, Dict[str, type]] = {
DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: {
DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY: Callable,
DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY: Callable},
DataNodeConfig._STORAGE_TYPE_VALUE_SQL: {
DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY: Callable,
DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY: Callable}
}By the way, we could use it to check the types of all properties (both required and optional), not only for callables.
Then, this dictionary could be extended to enterprise.
Does it make sense?
There was a problem hiding this comment.
Yep, that's my initial intention with this refactoring. Trying to reuse the code for different types of instances. Though I'd say we should find a different ticket for that 😅
| DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: [ | ||
| ( | ||
| Callable, | ||
| [ | ||
| DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY, | ||
| DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY, | ||
| ], | ||
| ) | ||
| ], | ||
| DataNodeConfig._STORAGE_TYPE_VALUE_SQL: [ | ||
| ( | ||
| Callable, | ||
| [ | ||
| DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY, | ||
| DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY, | ||
| ], | ||
| ), | ||
| ], | ||
| } |
There was a problem hiding this comment.
-
We should also add all the properties to check. not only the callable.
-
The dictionary should be moved close to the definition of the properties, in the DataNodeCOnfig class.
-
The format seems strange to me. I would have used a
Dict[str, Dict[str, Any]]instead ofDict[str, List[Tuple[Any, List[str]]]]{ DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: { DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY: Callable, DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY: Callable }, DataNodeConfig._STORAGE_TYPE_VALUE_SQL: { DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY: Callable, DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY: Callable } }What do you think? Is there a reason to group the properties by type?
There was a problem hiding this comment.
Ah sure, well don't have much reason haha. Since the original code focused mainly on Callable, I just went along with that :D I have created a ticket to resolve this if you're okay with that since the changes are bigger than what we're having atm. https://github.com/orgs/Avaiga/projects/6/views/2?pane=issue&itemId=89479596
| config._sections[DataNodeConfig.name]["new"].storage_type = "generic" | ||
| config._sections[DataNodeConfig.name]["new"].properties = {"write_fct": lambda x: x, "read_fct": lambda y: y} | ||
| with pytest.raises(SystemExit): | ||
| Config._collector = IssueCollector() | ||
| Config.check() | ||
| assert len(Config._collector.errors) == 2 | ||
| expected_error_messages = [ | ||
| "`write_fct` of DataNodeConfig `new` must be populated with a Callable and not a lambda." | ||
| " Current value of property `write_fct` is <function TestDataNodeConfigChecker." | ||
| "test_check_callable_properties.<locals>.<lambda>", | ||
| "`read_fct` of DataNodeConfig `new` must be populated with a Callable and not a lambda." | ||
| " Current value of property `read_fct` is <function TestDataNodeConfigChecker." | ||
| "test_check_callable_properties.<locals>.<lambda>", | ||
| ] | ||
| assert all(message in caplog.text for message in expected_error_messages) | ||
|
|
There was a problem hiding this comment.
BTW, this deserves:
- an entry in the release notes as an improvement of the checkers for checking the property types.
- A new item in the list: https://docs.taipy.io/en/develop/userman/advanced_features/configuration/config-checker/
There was a problem hiding this comment.
The PR I have created for this topic also contains changes required to the doc. https://github.com/orgs/Avaiga/projects/6/views/2?pane=issue&itemId=89479596
Co-authored-by: Jean-Robin <jeanrobin.medori@avaiga.com>
| DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: [ | ||
| ( | ||
| Callable, | ||
| [ | ||
| DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY, | ||
| DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY, | ||
| ], | ||
| ) | ||
| ], | ||
| DataNodeConfig._STORAGE_TYPE_VALUE_SQL: [ | ||
| ( | ||
| Callable, | ||
| [ | ||
| DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY, | ||
| DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY, | ||
| ], | ||
| ), | ||
| ], | ||
| } |
toan-quach
deleted the
feature/#521-refactor-code-for-databricks-integration
branch
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents