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
Swift: Support EnumContent in models-as-data #13795
base: main
Are you sure you want to change the base?
Conversation
| // model to allow data flow through `signum()` as though it were an identity function, for the benefit of testing flow through optional chaining (`x?.`). | ||
| ";Int;true;signum();;;Argument[-1];ReturnValue;value", | ||
| // test Enum content in MAD | ||
| ";;false;mkMyEnum2(_:);;;Argument[0];ReturnValue.EnumElement[mySingle:0];value", |
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 pondering whether the MAD syntax should contain the enum type as well, e.g. EnumElement[MyEnum,mySingle:0] here. In a way it would be more precise, but also more fiddly to specify. There isn't really much ambiguity anyway, because the type is effectively determined by the rest of the MAD signature - though QL performance may be somewhat affected by potential extra rows mid compute.
@MathiasVP do you have an opinion on this?
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.
The dataflow library actually does something like this internally for performance reasons. I'd prefer not including the type as part of the syntax since it increases the chances of us getting a row wrong when we write MaD models
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.
While we're on the top of syntax here, do you think the name EnumElement[...] is best, or perhaps EnumField[...], EnumValue[...] or EnumContent[...]. The documentation appears to call them "Associated Values" (where "Enumeration Values" mean the element name itself).
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 actually not sure why we shouldn't just support this via the same syntax we use for fields. Is it really important whether it's a field or an enumeration value?
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.
Fields have names, enum associated values do not. In a way they're perhaps closer to a tuple (which we don't have MaD syntax for as yet).
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 see what you mean. I was thinking of examples like:
enum Season {
case spring, summer, autumn, winter
}and considered whether we could just write .spring for this enum value. But we do indeed need special syntax to handle something like:
enum MyEnum { case mySingle(Int) }My vote is on EnumElement since that matches what we're doing for ArrayContent.
We have
EnumContentin Swift dataflow now. It's not supported in models-as-data. This PR addresses this, and also adds proper flow through!for unwrapping optional content (which is a kind ofEnumContent).The latter part may well affect other tests I haven't run locally so I'm relying on the PR checks to catch that. I also will do a DCA run once the tests are passing.