Passes end to end test an example opex file #1
Conversation
bdgregg
left a comment
bdgregg
left a comment
There was a problem hiding this comment.
@ojas-uls-dev in my look over, I did not see anything that stood out immediately as an issue. For a complete understanding of the logic of the code, I would need to see a demonstration of how this all works. I am excited to see the in roads into OPEX and look forward to see how to use this code.
ctgraham
left a comment
ctgraham
left a comment
There was a problem hiding this comment.
Started a read-through and then got hijacked for end-of-day. Incomplete feedback, but I wanted to make it available as a start.
| identifier_types: list[Optional[str]] = None | ||
| descriptive_metadata: etree._ElementTree = None | ||
|
|
||
| def as_xml_fragments(self): |
There was a problem hiding this comment.
Please add method/function documentation for purpose, input, output and side effects. For example, as I began reading this method I was surprised to see print calls, making me do a quick check for a return to figure out if the print was an output or a side effect.
|
|
||
| @staticmethod | ||
| def find(tree, name): | ||
| tag = tree.find(f".//opex:{name}", namespaces = OpexXmlHelper.ns_dict) |
There was a problem hiding this comment.
This seems like a risky assumption to assert that the first element in the tree is the match you are looking for. What constrains this so that someone doesn't instantiate a OpexXmlHelper at the root level and ask for .find('Type')? Is this a History > Events > Event > Type, or a Relationships > Relationship > Type? What about repeated instances of Event and Relationship elements?
There was a problem hiding this comment.
The ".//" recursive search was an ad hoc fix for elements not showing up. Switched to using explicit paths
3 participants
example file used for testing is available in SYSDEV-3051
Basic architecture is