Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kinow opened this issue Jan 18, 2022 · 4 comments
Open

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

kinow opened this issue Jan 18, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@kinow
Copy link
Member

kinow commented Jan 18, 2022

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().

@kinow kinow added the bug Something isn't working label Jan 18, 2022
@mr-c
Copy link
Member

mr-c commented Jan 18, 2022

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

@kinow
Copy link
Member Author

kinow commented Jan 18, 2022

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 👍

@mr-c mr-c changed the title New translator must translate WDL's sub as replaceAll translate WDL's sub() as replaceAll() not replace() Feb 16, 2022
@mr-c mr-c changed the title translate WDL's sub() as replaceAll() not replace() translate WDL's sub() as replaceAll() equivalent not replace() Feb 16, 2022
@mr-c
Copy link
Member

mr-c commented Feb 16, 2022

  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.

    return f"{sub_expr}.replace({arg_string_expr}, {arg_sub_expr}) "

  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
Copy link
Member Author

kinow commented Feb 16, 2022

Alas replaceAll is not in ECMAScript 5.1

😮 thought it was available in that spec. Sounds like the global flag is the way to go then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants