-
Notifications
You must be signed in to change notification settings - Fork 357
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
WX-1461 Remove womtool upgrade
command
#7382
Conversation
@@ -63,7 +58,7 @@ object LinkedGraphMaker { | |||
// we want to start the cycle with the edge "a -> b" | |||
val edgeDict: Map[String, String] = | |||
cycle.value.edges.map { case graph.EdgeT(from, to) => | |||
nodeName(from) -> nodeName(to) | |||
from.value.toString -> to.value.toString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These error messages for cyclical graphs used to look like
call a5.add5 {
complete with mismatched {
because nodeName()
wrote out the whole call and took the first line. Now looks like
Call "a5.add5"
so hardly any difference and this allows us to drop the entire WdlWriter[WorkflowGraphElement]
and all of its subtypes.
private def attributesToString: String = attributes.toWdlV1 | ||
// Yes, it's sad that we need to put ${} here, but otherwise we won't cache hit from draft-2 command sections | ||
// Re-writes e.g. `echo $((5 + ~{x.a}))` commands to `echo $((5 + ${x.a}))` so they evaluate to equal | ||
override def toString: String = "${" + s"$attributesToString${expression.expressionElement.toWdlV1}" + "}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is worth it or if I even agree with the original decision, but adjudicating that seemed out of scope for this PR. It does mean we have to retain the ability to write any expression element.
@@ -18,7 +18,7 @@ import wdl.model.draft3.graph.ExpressionValueConsumer.ops._ | |||
import wdl.model.draft3.graph.expression.WomExpressionMaker.ops._ | |||
import wdl.transforms.base.linking.expression._ | |||
import wdl.transforms.base.wdlom2wdl.WdlWriter.ops._ | |||
import wdl.transforms.base.wdlom2wdl.WdlWriterImpl._ | |||
import wdl.transforms.base.wdlom2wdl.WdlWriterImpl.{expressionElementWriter, placeholderAttributeSetWriter} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a workflow thing that made it easier for me to see what is really being used or not.
testFiles.foreach { file => | ||
it should s"write a file that re-evaluates to the same case classes for ${file.name}" in { | ||
|
||
val model: Checked[FileElement] = (fileToAst andThen wrapAst andThen astToFileElement).run(file) | ||
model match { | ||
case Right(wdlModel) => | ||
val newModel = (stringToAst andThen wrapAst andThen astToFileElement).run( | ||
FileStringParserInput(wdlModel.toWdlV1, file.name) | ||
) | ||
|
||
// Scala case class deep equality is so nice here | ||
newModel.map(stripSourceLocations) shouldEqual model.map(stripSourceLocations) | ||
case Left(_) => fail("Could not load original") | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remain proud of this code, which...
- parses a WDL on disk to classes
- writes out the classes with the WDL writer
- re-parses the written-out version
- compares the new classes with those in step (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much red, you love to see it.
CHANGELOG.md
Outdated
@@ -2,6 +2,10 @@ | |||
|
|||
## 87 Release Notes | |||
|
|||
### `upgrade` command removed from Womtool | |||
|
|||
Womtool previously supported a `womtool upgrade` command for upgrading draft-2 WDLs to 1.0. With WDL 1.1 taking over as the latest supported version, this functionality is retiring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to hedge this a little more, like "With WDL 1.1 soon to become the latest supported version"
Also removes large chunks of the WDL writer that were only used for very specific error messages.