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

Struct literal - Revist #286

Closed
wants to merge 4 commits into from
Closed

Conversation

patmagee
Copy link
Member

I have modified my original Struct Literal PR after rebasing on master once the object removal PR had been merged. I have removed all references to object literal notation and replaced it with struct literal as requested by @cjllanwarne .

Changes

Currently, there is no such thing as a Struct Literal, instead we use the Object literal notation and hope that the engine conversion does not mess up any of the types. On top of objects being deprecated in the near future, there is also the added problem that there could be a loss of information if an object literal is the first cast to an object, before being cast to the declaring type.

To address this, I propose the idea of Struct Literals, which as previously discussed here #201

I chose to go with a simple annotation for defining struct literals that should both look familiar and also not look too ugly when implementing (since it is based on python constructors)

Here is an example:

struct Person {
  Int age
  String name
}

...

Person a = Person( age=30, name="Bill" )

Open Questions for debate:

  1. Recent changes to the spec introduced the ability to use Map Literals to create a struct or allow map literals to be cast to a struct. My question is, with the implementation of struct literals, should this be allowed or encouraged? To me it makes sense to allow casting of a map to specific Struct type, but what do you guys think

@cjllanwarne
Copy link
Contributor

Re the open question - IMO I think we can state that it's possible if a map happens to be (a) a valid WDL map and (b) the right shape, but in our examples we should always use struct literals to demonstrate creation of structs

@patmagee
Copy link
Member Author

@cjllanwarne I agree, I changed a few map literal casts to simply structs

@DavyCats
Copy link
Contributor

DavyCats commented Mar 5, 2019

👍
I did spot one typo, but I guess I'm too late with this now 😛
It isn't a big deal anyway:
[...] which enforces the typing of all of is assigned parameters. on line 2096/2097 should be
[...] which enforces the typing of all of its assigned parameters.

@geoffjentry
Copy link
Member

👍

1 similar comment
@orodeh
Copy link
Contributor

orodeh commented Mar 5, 2019

👍

@aednichols
Copy link
Contributor

👍

We should fix Person(name="Harry", age=11} soon after; the other examples and prose description make the typo not a blocker.

@geoffjentry
Copy link
Member

@patmagee considering some of our more eagle eye voters are starting to pick up on typos and such, it might be worth pulling this back and then re-launching the vote


#Simple case
File fastq_1
File fastq_2
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also a typo presumably? I think it should either be (a) assigned a value (even File fastq2 = ... as a placeholder) or put into an input { } section

@cjllanwarne
Copy link
Contributor

One more potential typo for a fixup, but otherwise this looks great - and thanks for taking care of those option literals! 👍

@geoffjentry
Copy link
Member

@patmagee Voting has passed however it's clear that there are multiple typos here. Would you rather go ahead and merge in & followup w/ a cleanup PR, or would you rather clean it up & resubmit?

@patmagee
Copy link
Member Author

@geoffjentry I am notorious for my typos, this is quite bad. I personally think I should create a separate PR, closing this one, and we can merge that one. I would rather not include any errors that might be missed

@patmagee patmagee closed this Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants