#1248 Specify define xml path in operations#1649
Conversation
alexfurmenkov
requested review from
RamilCDISC,
SFJohnson24 and
gerrycampion
and removed request for
gerrycampion
alexfurmenkov
changed the title
| "-dxp", | ||
| "tests/resources/CoreIssue1248/define_subfolder/define.xml", | ||
| "-dp", | ||
| "tests/resources/CoreIssue1248/relrec.json", |
There was a problem hiding this comment.
Better would be to use excel dataset here instead of json. The PR updates are for excel dataset. Using json will not test the updated changes.
There was a problem hiding this comment.
we should add a test with no -dxp flag supplied to verify the case of getting define from dataset path also.
There was a problem hiding this comment.
This will not work without the -dxp flag until changes are made to ExcelDataService related to comment
or changes to self.rule_processor.perform_rule_operations call should be made, so dataset_metadata.full_path containing in our case relrec.xpt value will pass something else
| filename=dataset_name, | ||
| full_path=dataset_name, | ||
| full_path=self.dataset_path, | ||
| # full_path=dataset_name, |
There was a problem hiding this comment.
Could you please remove this old commented out code?
There was a problem hiding this comment.
I’ll revert it back to full_path=dataset_name for now.
Ideally, it should be full_path=self.dataset_path so the name matches and the report shows the actual path.
However, changing this will break the current code because get_dataset expects a dataset name, while all service clients are currently sending full_path.
Possible solutions:
- Detect the service type and send the filename field instead of
full_path. This would work but doesn’t feel very elegant. - Change
get_datasetto accept a dataset metadata object instead of a path parameter. Then each service can decide what field from the metadata it needs to retrieve the dataset.
There was a problem hiding this comment.
The AC in the attached issue requires solution of the bug in excel dataset use path. i think we should resolve the issue in this PR before merge. You can discuss with @SFJohnson24 and @gerrycampion to decide what solution to use.
| else os.path.join(self.params.directory_path, DEFINE_XML_FILE_NAME) | ||
| ) | ||
| if not os.path.exists(define_path): | ||
| raise FileNotFoundError("Define XML file %s not found", define_path) |
There was a problem hiding this comment.
This string formatting is incorrect. Use f"Define XML file {define_path} not found" instead.
…a' into 1248-fix-define-variable-metadata
| define_path = ( | ||
| self.params.define_xml_path | ||
| if self.params.define_xml_path | ||
| else os.path.join(self.params.directory_path, DEFINE_XML_FILE_NAME) | ||
| ) | ||
| if not os.path.exists(define_path): | ||
| raise FileNotFoundError(f"Define XML file {define_path} not found") | ||
| define_contents = self.data_service.get_define_xml_contents( | ||
| dataset_name=os.path.join(self.params.directory_path, DEFINE_XML_FILE_NAME) | ||
| dataset_name=define_path | ||
| ) | ||
| define_reader = DefineXMLReaderFactory.from_file_contents(define_contents) |
There was a problem hiding this comment.
This chunk should be moved to a BaseOperation function so that it can be reused
3 participants
Added define xml path parameter to operation.
Found an issue in ExcelDataService ruining define.xml loading, because we use one path to look for both data and define.xml. Also, when we use ExcelDataService, dataset metadata doesn't contain real path to the *.xlsx file.