Optionals methods and pair literals #98

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
4 participants
Contributor

cjllanwarne commented Mar 3, 2017

Doc changes for the upcoming Cromwell 25 release.

+Pair values can be specified using another Python-like syntax, as follows:
+
+```
+Pair[Int, String] twenty_threes = (23, "twenty-three")
@ruchim

ruchim Mar 29, 2017

Contributor

not sure if the name "twenty_threes" was intentional, or it was meant to be "twenty_three"

@vdauwera

vdauwera Apr 1, 2017

Collaborator

Considering the Pair contains two ways to express the quantity in question, I'm going to assume it was intentionally pluralized.

@cjllanwarne

cjllanwarne Apr 3, 2017

Contributor

It's been a while since I wrote it but I'm happy to agree with @vdauwera that this is indeed a pair of twenty-threes and will therefore leave it as-is

SPEC.md
+
+:pig2: [Cromwell supported](https://github.com/broadinstitute/cromwell#wdl-support) :white_check_mark:
+
+Given an array of optional values, `select_all` will select the first defined value and return it. Note that this is a runtime check and you should be confident that a defined value will exist: if no defined value is found when select_first runs, the workflow will fail.
@ruchim

ruchim Mar 29, 2017

Contributor

replace select_all with select_first

@vdauwera

vdauwera Apr 1, 2017

Collaborator

"you should be confident that a defined value will exist" -> "requires that at least one defined value exists at the time the check is made" ?

SPEC.md
+
+:pig2: [Cromwell supported](https://github.com/broadinstitute/cromwell#wdl-support) :white_check_mark:
+
+Given an array of optional values, `select_all` will select only those elements which are defined.
@ruchim

ruchim Mar 30, 2017

Contributor

This is the same as line 2593.

Contributor

ruchim commented Mar 30, 2017 edited by briandmaher

👍 with some small adjustments

Approved with PullApprove

Collaborator

katevoss commented Mar 31, 2017

@vdauwera could you review this and merge if it's ready to go? This was forgotten last Cromwell release, whoops!

@vdauwera

There are a couple of things to address between my review and Ruchi's

+Pair values can be specified using another Python-like syntax, as follows:
+
+```
+Pair[Int, String] twenty_threes = (23, "twenty-three")
@vdauwera

vdauwera Apr 1, 2017

Collaborator

Considering the Pair contains two ways to express the quantity in question, I'm going to assume it was intentionally pluralized.

@@ -2488,7 +2502,7 @@ Example 2:
}
output {
- File outputFile = ${output_file_name}
+ File outputFile = output_file_name
@vdauwera

vdauwera Apr 1, 2017

Collaborator

Wait, is this a new syntax for outputs? And does this mean "you can write it either way" or "you have to write it the new way"?

Apologies if this was specified in release notes that I failed to read.

@cjllanwarne

cjllanwarne Apr 3, 2017

Contributor

AFAIK, this has always been the correct syntax for outputs:

  • output_file_name is a variable, and you're assigning its value to outputFile.
  • ${output_file_name} is a string interpolation that you can use inside a command block. I don't know that it would work outside a command block and would argue that it shouldn't.
  • You might be able to do something like = "${output_file_name}" but this is a needless indirection. You'd be saying "insert this String into another String, and then use that". It works, but you might as well use the original String in the first place
SPEC.md
+
+:pig2: [Cromwell supported](https://github.com/broadinstitute/cromwell#wdl-support) :white_check_mark:
+
+Given an array of optional values, `select_all` will select the first defined value and return it. Note that this is a runtime check and you should be confident that a defined value will exist: if no defined value is found when select_first runs, the workflow will fail.
@vdauwera

vdauwera Apr 1, 2017

Collaborator

"you should be confident that a defined value will exist" -> "requires that at least one defined value exists at the time the check is made" ?

cjllanwarne was assigned by vdauwera Apr 1, 2017

Collaborator

vdauwera commented Apr 1, 2017

Back to @cjllanwarne

Contributor

cjllanwarne commented Apr 3, 2017

@vdauwera @ruchim @katevoss for your consideration: 0514cec

@cjllanwarne cjllanwarne assigned vdauwera and unassigned cjllanwarne Apr 3, 2017

Contributor

ruchim commented Apr 6, 2017

Saw the changes of the second commit, looks beautiful 💯

Changes made; not rejoined

@cjllanwarne cjllanwarne merged commit c84ade7 into develop Apr 10, 2017

1 check was pending

code-review/pullapprove Approval required by 1 of: geoffjentry, Horneth, jsotobroad, katevoss, kcibul, kshakir, mcovarr
Details

cjllanwarne deleted the cjl_C25_update branch Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment