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

Optional case counts, PE, and SR file inputs for SingleSample workflow #89

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

mwalker174
Copy link
Collaborator

Lets users bypass Module00a in the SingleSample workflow.

Copy link
Member

@cwhelan cwhelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, nice addition.

I have one tiny complaint about the preexisting but related code: one thing that always trips me up when I follow the logic around whether 00a gets run is that running the callers in 00a is triggered by the docker string inputs being defined, and that we purposefully set them to undefined by setting them to NONE_STRING_, which should never be defined. It makes me want to rename NONE_STRING_ to UNDEFINED_STRING_ to help me remember what it is. I know we follow a similar pattern in other WDL files, though, so if other people don't have the same problem remembering it that I do then feel free to ignore this.

@mwalker174
Copy link
Collaborator Author

mwalker174 commented Dec 16, 2020

Thanks @cwhelan. I chose NONE_STRING_ because None is the null type identifier for WDL (see openwdl/wdl#263). I'm not opposed to UNDEFINED_STRING_ either, though it would be better if we could find a way to avoid using this hack in the first place.

@mwalker174 mwalker174 merged commit d322dc4 into master Dec 16, 2020
@mwalker174 mwalker174 deleted the mw_single_sample_evidence_inputs branch December 16, 2020 16:46
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.

2 participants