common-workflow-lab / wdl-cwl-translator

A translator from WDL to CWL v1.2
Apache License 2.0
26 stars 8 forks source link

translate WDL's sub() as replaceAll() equivalent not replace() #128

Open kinow opened 2 years ago

kinow commented 2 years ago

From WDL spec:

this function replaces all non-overlapping occurrences of pattern in input by replace

But I think what we have in the get_expr_apply is a call to .replace() which only replaces one occurrence (different than the Python function with same name!)

Instead I think we must use JS' replaceAll().

mr-c commented 2 years ago

Good catch! We should add a test case with data that would demonstrate this

kinow commented 2 years ago

This workflow is using it to replace spaces by underscores (for filename): https://github.com/broadinstitute/viral-pipelines/blob/f009d869ddb75770ae0aa69d9dd8bcaf1390502e/pipes/WDL/tasks/tasks_reports.wdl#L367

That workflow has a lot more than what we need, so we can just extract the test case for a string with spaces, then replace space by _, and that's that :+1:

mr-c commented 2 years ago
  1. We are already not passing the pattern correctly to replace as the openWDL 1.1 spec says

    pattern is a regular expression that will be evaluated as a POSIX Extended Regular Expression (ERE).

    so we should be wrapping the replace string in /…/ to mark them as JS RegExp objects. https://github.com/common-workflow-lab/wdl-cwl-translator/blob/63d60b51421a68b819411e36ed47a1df547db411/wdl2cwl/main.py#L852

  2. Alas replaceAll is not in ECMAScript 5.1. Luckily the specification for replace() reminds us that

    If searchValue.global is true, then search string for all matches of the regular expression searchValue.

    We can set the global flag by formatting the pattern string like /{arg_string_expr}/g with the g suffix.

kinow commented 2 years ago

Alas replaceAll is not in ECMAScript 5.1

:open_mouth: thought it was available in that spec. Sounds like the global flag is the way to go then :+1: