984: define subversion workaround#1697
Conversation
alexfurmenkov
changed the title
| original_version = match.group(2) | ||
| xml_content = re.sub(define_version_pattern, r"\g<1>2.1\3", xml_content) | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode="w+", suffix=".xml", delete=False) as tmp: |
There was a problem hiding this comment.
We should keep delete = True so temp files are cleaned up after done use
| return reader | ||
|
|
||
| @classmethod | ||
| def from_file_contents( |
There was a problem hiding this comment.
Normalization is only added to from filename but from_file_contents is missing it.
|
|
||
| @classmethod | ||
| def from_filename(cls, filename: str): | ||
| """ |
There was a problem hiding this comment.
Could you please restore the docstring on this function?
| DefineXMLReader20, | ||
| DefineXMLReader21, | ||
| ) | ||
| _DEFINE_VERSION_RE = re.compile(r'(?<=def:DefineVersion=")2\.1\.\d+(?=")') |
There was a problem hiding this comment.
This regex is too brittle for a define xml. The rest of the factory uses ElementTree to inspect actual attribute Namespaces. Do you think that would be a better approach?
| reader: BaseDefineXMLReader = define_xml_reader_class( | ||
| cache_service_obj, study_id, data_bundle_id | ||
| if isinstance(file_contents, bytes): | ||
| file_contents: str = file_contents.decode("utf-8") |
There was a problem hiding this comment.
Fixing to 'UTF-8' can introduce regression here. ElementTree.Fromstring handles the encodings better. If you are sure that Define-XML will always be 'UTF-8' please let me know and then you can ignore this comment.
# Conflicts: # cdisc_rules_engine/services/define_xml/define_xml_reader_factory.py
| ElementTree.parse(filename).getroot() | ||
| ) | ||
| reader: BaseDefineXMLReader = define_xml_reader_class() | ||
| reader._odm_loader.open_odm_document(filename) |
There was a problem hiding this comment.
The change here changes the behavior of from_filename to read using a string instead of odm_loader and also will hardcode the define xml encoding to UTF-8. @gerrycampion please let us know if you agree with this change.
There was a problem hiding this comment.
changed to read bytes instead, so it will treat xml encoding with respect
SFJohnson24
left a comment
SFJohnson24
left a comment
There was a problem hiding this comment.
this PR correctly provides work around for define subversions. Passes high level review. Tested with correct validation execution and reporting for v2.0, 2.0.0, 2.1.11, 2.1.0 define
3 participants
changing 2.1.* subversions to 2.1 in define.xml before creating odm reader.
original version stored in DefineXmlReader, accessed in get_define_version to be shown in report