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

WX-964 suffix() #7363

Merged
merged 7 commits into from
Feb 9, 2024
Merged

WX-964 suffix() #7363

merged 7 commits into from
Feb 9, 2024

Conversation

jgainerdewar
Copy link
Contributor

Adds support for suffix engine function. This function was added in WDL 1.1, so Cromwell support has been added for WDL versions starting with our 1.1 development branch.

WDL Version Version declaration Codename in Cromwell suffix support
draft-2 (none) draft-2 No
1.0 1.0 draft-3 (AKA arcadia... in my heart) No
1.1 development-1.1 biscayne Yes
Next (1.2 or 2.0) development cascades Yes

@jgainerdewar jgainerdewar requested a review from a team as a code owner January 31, 2024 20:50
expressionValueEvaluator
)
) { (suffix, arr) =>
EvaluatedValue(WomArray(arr.value.map(v => WomString(v.valueString + suffix.value))), Seq.empty).validNel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the part that actually does the suffix-ing

Copy link
Collaborator

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

Everything in /cascades/ and /biscayne/ looks reasonable to me, not sure about the purpose of modifying /draft-3/ (though I do see a test, that presumably passes)

Comment on lines +168 to +171
final case class Suffix(suffix: ExpressionElement, array: ExpressionElement) extends TwoParamFunctionCallElement {
override def arg1: ExpressionElement = suffix
override def arg2: ExpressionElement = array
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

draft-3 is not supposed to support suffix, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but all the other post-1.0 engine functions (ex. min, max, sep) have their case classes in here so I followed that convention. There aren't equivalent files in the other language version packages, though I can make that change if desirable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like those functions may have been put in there before a next language version was declared and/or before we had our current organization scheme. I do think that separating them out (both current and the new addition) would be optimal, though maybe that's a separate ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed off-github and decided to leave this as-is.

Copy link
Contributor

@THWiseman THWiseman left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding the version table to the description, too.

@jgainerdewar jgainerdewar merged commit a39eabe into develop Feb 9, 2024
33 of 34 checks passed
salonishah11 pushed a commit that referenced this pull request Feb 15, 2024
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.

None yet

3 participants