Implementing better error messages #83
Conversation
Signed-off-by: Christian Henkel <[email protected]>
Signed-off-by: Christian Henkel <[email protected]>
Signed-off-by: Christian Henkel <[email protected]>
Signed-off-by: Christian Henkel <[email protected]>
| """Set the xml_element this object was made from""" | ||
| self.xml_tree = xml_tree | ||
|
|
||
| def get_xml_tree(self) -> Optional[ET.Element]: |
There was a problem hiding this comment.
I wonder if we should call this get_xml_element, though it might not really be consistent with the from_xml_tree method.
| @@ -75,8 +75,10 @@ def _interpret_type_from_comment_above( | |||
| return None | |||
| return type_match.group(1), type_match.group(2) | |||
|
|
|||
| @staticmethod | |||
| def from_xml_tree(xml_tree: ET.Element, comment_above: Optional[str] = None) -> "ScxmlData": | |||
| @classmethod | |||
There was a problem hiding this comment.
I see there is a reason for not using the from_xml_tree_impl pattern as for the other classes, but nevertheless you could override the from_xml_tree method, and additionally I do not see the reason for switching from the staticmethod to the classmethod, unless we decide to do this consistently.
Finally, I am wondering if we could take the generalization one step further, by providing an optional kwargs style arguments list in from_xml_tree and from_xml_tree_impl, like explained here: https://www.geeksforgeeks.org/args-kwargs-python/
| def get_data_entries(self) -> Optional[List[ScxmlData]]: | ||
| def get_data_entries(self) -> List[ScxmlData]: | ||
| if self._data_entries is None: | ||
| return [] |
There was a problem hiding this comment.
I would rather enforce the self._data_entries to be a list (that is already done in check_validity, but could be enforced in the __init__, too!
| @@ -42,8 +42,8 @@ class ScxmlParam(ScxmlBase): | |||
| def get_tag_name() -> str: | |||
| return "param" | |||
|
|
|||
| @staticmethod | |||
| def from_xml_tree(xml_tree: ET.Element) -> "ScxmlParam": | |||
| @classmethod | |||
There was a problem hiding this comment.
As before, does it make sense to use the classmethod decorator for static methods not accessing the actual cls value?
| @@ -93,30 +97,45 @@ def read_value_from_xml_child( | |||
| xml_child = xml_tree.findall(child_tag) | |||
| if xml_child is None or len(xml_child) == 0: | |||
| if not none_allowed: | |||
| print(f"Error: reading from {xml_tree.tag}: Cannot find child '{child_tag}'.") | |||
| log_error( | |||
There was a problem hiding this comment.
This is neat! We should make a pass over the whole AS2FM and make sure we use it consistently (can also be done in another PR, in the future)! :)
| for child in tag_children: | ||
| print(f"\t- {child.tag}") | ||
| log_error(xml_tree, f"\t- {child.tag}") |
There was a problem hiding this comment.
This was supposed to be a longer error message, containing also the list of children... Maybe we can simply remove the for loop and add the n. of children in the error message
| @@ -76,12 +76,13 @@ def _assemble_message(severity: Severity, element: "lxml.etree._Element", messag | |||
| "The element must have a filepath attribute. This is set by " | |||
There was a problem hiding this comment.
I have to admit that the implementation of this function slipped completely my attention! If I knew it existed, I would have started using it so long ago! :)
No description provided.