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

EC #7694

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

EC #7694

wants to merge 13 commits into from

Conversation

tarrinneal
Copy link
Contributor

No description provided.

@tarrinneal tarrinneal changed the title Top Secret Pigeon Feature EC Sep 27, 2024
@tarrinneal
Copy link
Contributor Author

@hellohuanlin any chance you can look at the generated swift code and let me know if there is a more idiomatic way to do this?

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

Only reviewed swift syntax. But I may need a better understanding for a more in-depth review (is there a design doc of what this is for?)

func onCancel(withArguments arguments: Any?) {}
}

class PigeonEventSink<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is just a 1-line wrapper? Can we just inline the calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "inline" specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we directly use the wrapped type rather than using this wrapper?

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, the purpose of this wrapper is to enforce type safety on the return value

Copy link
Contributor

Choose a reason for hiding this comment

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

By type safety you mean the generic type T in the success(_ value:) call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, so I see what you mean, the error method isn't needed. I'm not sure if this is it's final form, but if it is, I'll remove it.

@tarrinneal
Copy link
Contributor Author

Only reviewed swift syntax. But I may need a better understanding for a more in-depth review (is there a design doc of what this is for?)

Syntax is all I was really hoping for :) Thank you. Just wanted to make sure there wasn't a more idiomatic way to handle the class structure and inheritance and such.

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.

2 participants