Allow for meta and parameter_meta blocks in the workflow section #53
Conversation
| @@ -286,6 +286,26 @@ object AstTools { | |||
|
|
|||
| def terminalMap(ast: Ast, source: WdlSource) = (findTerminals(ast) map {(_, source)}).toMap | |||
|
|
|||
| def wdlSectionToStringMap(ast: Ast, node: String, wdlSyntaxErrorFormatter: WdlSyntaxErrorFormatter): Map[String, String] = { | |||
There was a problem hiding this comment.
this was refactored nearly verbatim from Task
There was a problem hiding this comment.
Refactored or copy/pasted? Is it DRYable? My bad. I'm a fool.
There was a problem hiding this comment.
The whole point of moving it here was to not copy paste
| case a if a.isEmpty => Map.empty[String, String] | ||
| case a if a.size == 1 => | ||
| // Yes, even 'meta {}' and 'parameter_meta {}' sections have RuntimeAttribute ASTs. | ||
| // In hindsight, this was a poor name for the AST. |
There was a problem hiding this comment.
ToL how hard would this be to fix now that we're actually supporting these things?
There was a problem hiding this comment.
Touching on our convo this morning, probably not hard but I'd be real leery of hosing things up.
The name definitely is confusing, if it weren't for that comment I'd not have realized I needed this bit.
There was a problem hiding this comment.
Are you concerned the existing tests wouldn't notice if you did in fact hose this up?
There was a problem hiding this comment.
My ToL 2c, this seems possibly worth hosing-up (but then obviously also fixing-up), if possible?
There was a problem hiding this comment.
Looks like the offender is:
$kv = :identifier :colon $e -> RuntimeAttribute(key=$0, value=$2)
... which is causing all the key/value pairs to create RuntimeAttribute ASTs. You could rename it something more generic like KeyValuePair, and update all wdl4s builders to search for that instead of RuntimeAttribute. Hopefully...
There was a problem hiding this comment.
Alternatively, since these are (should) be constants, we could say the meta block is actually a list of identifier -> String?
There was a problem hiding this comment.
Discussed earlier in slack with @mcovarr - my concern was that the original author of that comment is someone I would have expected to fix that if it wasn't a big deal. I can see reasons why he wouldn't have that don't fit that model however.
There was a problem hiding this comment.
Yeah. My refined ToL preference would actually be, disallow expressions in meta blocks... but (shrug)
There was a problem hiding this comment.
I believe @cjllanwarne is correct. As the underlying AST structure is tangential to this change and because I'd like to get this change in the release (and I don't have faith I'd manage to get both squared away) I filed wdl issue 73.
I can't imagine there's a reason to not change the grammar for that (in fact it would more closely adhere to my reading of the spec) but we shouldn't change functionality like that w/o PO discussion, etc.
|
👍 |
|
👍 with maybe a small test to make sure that the pairs make it to the |
|
@Horneth I'll double check but I don't think the one test I modified would pass if that weren't the case |
|
@Horneth Double checked - unless I misunderstood you that's explicitly checked by the one key test as that content is coming from the workflow/task objects |
Closes WDL issue #67. PRs for related repos (WDL, wdltool & cromwell) to follow after this is merged