Made 'size' accept empty file? values #2422

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

cjllanwarne commented Jul 5, 2017

No description provided.

Contributor

mcovarr commented Jul 6, 2017 edited by briandmaher

👍 aside from the whole "unfilled optionals of types that could be coerced to files have size zero" business 😛

Approved with PullApprove

@mcovarr

mcovarr approved these changes Jul 6, 2017

project/Dependencies.scala
@@ -2,7 +2,7 @@ import sbt._
object Dependencies {
lazy val lenthallV = "0.25"
- lazy val wdl4sV = "0.13"
+ lazy val wdl4sV = "0.14-c3be413-SNAP"
@mcovarr

mcovarr Jul 6, 2017

Contributor

you'll need to publish this or something like it for green builds

@cjllanwarne

cjllanwarne Jul 10, 2017

Contributor

hmm, that's automatic when we push to WDL4S develop though, right?

@@ -122,11 +122,20 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions
override def size(params: Seq[Try[WdlValue]]): Try[WdlFloat] = {
def toUnit(wdlValue: Try[WdlValue]) = wdlValue flatMap { unit => Try(MemoryUnit.fromSuffix(unit.valueString)) }
+ def optionalSafeFileSize(value: WdlValue): Try[Double] = value match {
+ case f: WdlFile => Try(buildPath(f.valueString).size.toDouble)
+ case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) map { coerced => buildPath(coerced.asInstanceOf[WdlFile].valueString).size.toDouble }
@mcovarr

mcovarr Jul 6, 2017

Contributor

rather than map and essentially duplicate the line above, you could flatMap and recurse

@mcovarr

mcovarr Jul 6, 2017

Contributor

i.e.

      case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) flatMap optionalSafeFileSize
@cjllanwarne

cjllanwarne Jul 10, 2017

Contributor

Oh I like that!

Contributor

mcovarr commented Jul 6, 2017

oh and some tests wouldn't hurt 😛

Member

geoffjentry commented Jul 6, 2017

Including centaur :)

Contributor

cjllanwarne commented Jul 11, 2017

Added some tests. @geoffjentry I think these unit tests are comprehensive enough without (re-)testing that Cromwell wires variables into engine functions correctly in centaur. Hope you agree?

Member

geoffjentry commented Jul 11, 2017

@cjllanwarne My centaur comment was more that ideally we'd be testing every function in centaur. I'm sure this wouldn't be the first one we're slacking on, don't let me hold you up.

Contributor

cjllanwarne commented Jul 11, 2017

@geoffjentry I'll rephrase:

  • I agree that the size function's interaction with the outside world deserves a centaur test (and I believe it has one).
  • Since this PR only adds functionality that's entirely internal to Cromwell, I don't think (even in a perfect world) that we need another empty_optionals_size test in centaur to make sure that empty optionals give a size of 0?
Member

geoffjentry commented Jul 11, 2017

I'll file that under the "don't let me hold you up" part

Member

geoffjentry commented Jul 11, 2017 edited by briandmaher

@cjllanwarne realized you were probably looping back due to lack of 👍

Approved with PullApprove

cjllanwarne added some commits Jul 5, 2017

@cjllanwarne cjllanwarne Made 'size' accept empty files, Cromwell-side 8f3259e
@cjllanwarne cjllanwarne PR fixup 98c8c5a
@cjllanwarne cjllanwarne WDL4S bump 7c630f0
@cjllanwarne cjllanwarne WDL4S bump part II 1eb555a
@cjllanwarne cjllanwarne Bunch of extra tests for the ReadLikeFunctions e396db3
@cjllanwarne cjllanwarne Actually, it does need to accept Strings
3c8467a
@cjllanwarne cjllanwarne Hmm
e25e978
Contributor

cjllanwarne commented Jul 19, 2017

Will return...

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