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

Remove string interpolator options from development spec + add sep function #381

Merged
merged 9 commits into from
May 12, 2020

Conversation

illusional
Copy link
Contributor

@illusional illusional commented Apr 27, 2020

Motivation

Approach

Followed Adding WDL functions tutorial.

Checklist

  • Add appropriate test(s) to the automatic suite
  • Use make pretty to reformat the code with black
    • This is currently formatting more than just my change in StdLib.py - so I've disabled it.
    • This was being caused by a different version of black, upgrading solved this.
  • Use make check to statically check the code using Pyre and Pylint
  • Send PR from a dedicated branch without unrelated edits
  • Ensure compatibility with this project's MIT license

Calling the sep function inside an interpolated line (inside a command block) is failing if it's the only argument inside the block, eg:

I presume this might be a grammar thing, but I don't know how to solve it.

command <<<
    # FAILING
    echo ~{sep(",", ["value1", "value2", "value3"])}
    # PASSING
    echo ~{"" + sep(",", ["value1", "value2", "value3"])}

@coveralls
Copy link

coveralls commented Apr 27, 2020

Pull Request Test Coverage Report for Build 1830

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 96.871%

Files with Coverage Reduction New Missed Lines %
WDL/runtime/download.py 1 98.59%
WDL/runtime/workflow.py 1 97.02%
Totals Coverage Status
Change from base Build 1829: -0.03%
Covered Lines: 5697
Relevant Lines: 5881

💛 - Coveralls

@mlin
Copy link
Collaborator

mlin commented Apr 27, 2020

Thanks! miniwdl's 'development' grammar still supports the special 'sep' construct in interpolations, so that's the likely cause of the conflict. My understanding is that the old sep is penciled for removal too (openwdl/wdl#229) so this shouldn't be an issue in the long run, but lets carry on a discussion of how to sequence those changes on those PRs.

@mlin
Copy link
Collaborator

mlin commented Apr 27, 2020

Ok, clarifying my understanding, openwdl/wdl#229 both deletes the old sep= construct and adds the new sep() function. So if we expand this to accomplish the former, it wouldn't have other dependencies.

@illusional illusional changed the title Add sep function for joining array of strings Remove string interpolator options from development spec + add sep function Apr 28, 2020
@illusional
Copy link
Contributor Author

Thanks @mlin, I've realised I linked the wrong PR, and have fixed this. It was supposed to link to the "Clarify the expression placeholder and string interpolation behaviour" (openwdl/wdl#229) PR.

Per your suggestion and the now correctly linked PR, I've modified the grammar for the development spec to remove all string interpolator options. The implementation would have to remain for previous versions of the spec to work correctly.

I've added some tests into test_5stdlib.py and ran 2 / 3 cases in my test, but having some trouble with the one that has a command line binding, I get a docker mount error:

RuntimeError: docker task rejected, desired state shutdown: invalid mount config for type "bind": bind source path does not exist: /var/folders/jz/y9gqxt_s7jxcjkc26gr71ywr7zs5yz/T/miniwdl_test_stdlib_ys2hr_wk/20200428_104923_SepTest/command

@mlin
Copy link
Collaborator

mlin commented Apr 28, 2020

@illusional try setting environment TMPDIR=/tmp before the tests to get around that problem on macOS. It's because Docker for Mac has a whitelist of host paths that can be mounted into containers, and it doesn't include /var/folders unfortunately.

The travis build looks good and the coveralls is an unrelated stochastic fluctuation, nothing to worry about.

@mlin mlin added this to the WDL 2.0 milestone Apr 28, 2020
@mlin mlin added the interop Bears on spec compatibility label Apr 28, 2020
@illusional illusional marked this pull request as ready for review May 10, 2020 23:29
@illusional
Copy link
Contributor Author

Hey @mlin, I think this is good to go.

  • make pretty is showing warnings for unrelated lines, no warnings cover the changes I've made.
  • The make check fails with pyre: error: unrecognized arguments: --show-parse-errors. I get more unrelated pyre errors about cache files not existing.

But my tests run, so this should be okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Bears on spec compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants