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

Swift: Support EnumContent in models-as-data #13795

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 21, 2023

We have EnumContent in 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 of EnumContent).

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.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Jul 21, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner July 21, 2023 21:38
// 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",
Copy link
Contributor Author

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?

Copy link
Contributor

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 😂

Copy link
Contributor Author

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).

Copy link
Contributor

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?

Copy link
Contributor Author

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).

Copy link
Contributor

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.

@geoffw0 geoffw0 removed the no-change-note-required This PR does not need a change note label Jul 24, 2023
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.

None yet

2 participants