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

handle WdlPair - to fix #1703 #1704

Merged
merged 3 commits into from
Nov 30, 2016
Merged

handle WdlPair - to fix #1703 #1704

merged 3 commits into from
Nov 30, 2016

Conversation

delocalizer
Copy link
Contributor

JsArray is the simplest... but do you think JsObject with keys "left" and "right", or "0" and "1" would be better?
"left" and "right" would be consistent with the wdl4s code, but not extend very well to larger tuple types if you ever decide to go there.

@delocalizer
Copy link
Contributor Author

This was to fix #1703

@delocalizer delocalizer changed the title * handle WdlPair handle WdlPair - to fix #1703 Nov 24, 2016
@cjllanwarne
Copy link
Contributor

cjllanwarne commented Nov 29, 2016

Sorry that I missed this over the Thanksgiving break. This is awesome.

Personally I would favour a JsObject containing left and right members. I agree that this doesn't extend well to 3,4,5-tuples, etc, but I suspect for anything above 2 we'd want some sort of generic Tuple type which could have it's own rules for objectifying. In that case I could imagine a Tuple which happened to be length 2 might objectify differently from a Pair.... And IMO that'd be fine.

But I'm not the only one with an opinion, I'm sure! @kcibul, @geoffjentry?

@cjllanwarne
Copy link
Contributor

cjllanwarne commented Nov 29, 2016

Two other comments

  • Could you add a ticket to allow the same style of JSON to be used for Pair inputs (whatever we decide here re JsArrays vs JsObjects
  • Could you add a test case?

@geoffjentry
Copy link
Contributor

@cjllanwarne @delocalizer I don't have a strong opinion either way although I think left/right looks nicer.

I'm not concerned about expandability into tuples. Chris what you said is pretty much exactly what we laid out in the "but what if we want tuples?" discussion when talking about Pairs. My take on that is if that happens then the pair simply becomes semantic sugar over a tuple2 at which point we can map left/right to whatever representation a tuple2 uses.

@delocalizer
Copy link
Contributor Author

delocalizer commented Nov 29, 2016

@cjllanwarne I added a test - is that the kind of thing you were after?

@Horneth
Copy link
Contributor

Horneth commented Nov 29, 2016

FWIW I like the JsObject version better as well. Thanks for this !

@cjllanwarne
Copy link
Contributor

cjllanwarne commented Nov 29, 2016

Looks good. 👍

Approved with PullApprove

@geoffjentry
Copy link
Contributor

geoffjentry commented Nov 30, 2016

👍

Approved with PullApprove

@kshakir
Copy link
Contributor

kshakir commented Nov 30, 2016

Reminder, while some of the tests should work, the entire current Travis test suite will never pass due to #1717.

@geoffjentry geoffjentry merged commit 94eb009 into broadinstitute:develop Nov 30, 2016
@geoffjentry
Copy link
Contributor

Thanks @delocalizer !

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.

5 participants