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

Accept WDL file as first argument to support shebang. #164

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

prihoda
Copy link

@prihoda prihoda commented Jun 17, 2019

Since multiple runners are expected, it will be best to just provide the check/cromwell/... command when running the WDL file. This means that the shebang will essentially just create a shorthand from miniwdl COMMAND ./hello.wdl [ARGS...] to ./hello.wdl COMMAND [ARGS...].

Usage:

  1. Add #!/usr/bin/env miniwdl shebang line to your WDL file.

  2. Show help

./hello.wdl --help
# this translates to: miniwdl ./hello.wdl --help
  1. Check file
./hello.wdl check
# this translates to: miniwdl ./hello.wdl check
  1. Execute using cromwell
./hello.wdl cromwell name=MyName
# this translates to: miniwdl ./hello.wdl cromwell name=MyName
  1. Show normal help
miniwdl --help
# works as before, the new functionality is hidden:
# usage: miniwdl [-h] [--version] {check,cromwell} ...
#
# positional arguments:
#  {check,cromwell}
#    check           Load and typecheck a WDL document; show an outline with
#                    lint warnings
#    cromwell        Run workflow locally using Cromwell 40
#
# optional arguments:
#  -h, --help        show this help message and exit
#  --version         show miniwdl package version information
  1. Run non-existent file:
miniwdl ./nonExistent.wdl
# works as before - expects a command:
# usage: miniwdl [-h] [--version] {check,cromwell} ...
# miniwdl: error: argument command: invalid choice: './nonExistent.wdl' (choose from 'check', 'cromwell')

Introduced changes:
Checks if first argument passed is not "cromwell" or "check" and is a valid file path instead, which means shebang was used. In such case, add the file path as the first argparse argument and skip adding it in the subparsers.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 679

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.272%

Totals Coverage Status
Change from base Build 678: 0.0%
Covered Lines: 3280
Relevant Lines: 3407

💛 - Coveralls

@mlin
Copy link
Collaborator

mlin commented Jun 19, 2019

This is looking great! We should add some basic automated tests to prevent future subparser changes/refactoring from busting the shebang functionality.

I think the best way to set those up will be adding on to the existing bash-tap scripts, check.t and cromwell.t, probably a third .t file since the shebang script can invoke both 'check' and 'cromwell' (really cool touch!). The Makefile runs the .t scripts here. It may be tricky to figure out what to write on the shebang line for testing purposes, since we want it to invoke the development checkout of the miniwdl repo, not whatever released version pip has installed.

@kislyuk any advice on how/if argcomplete can do its thing via an executable shebang script (where the shebang invokes our python entry point) as contemplated here?

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

Successfully merging this pull request may close these issues.

3 participants