Skip to content

Add ChannelPipeline.SynchronousOperations.Position #3065

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

Merged
merged 11 commits into from
Feb 4, 2025

Conversation

0xTim
Copy link
Contributor

@0xTim 0xTim commented Jan 16, 2025

Adds a new ChannelPipeline.SynchronousOperations.Position type to fill a hole in the API that prevented non-sendable Channel handlers being added at a position to the pipeline via the synchronous operations.

@0xTim
Copy link
Contributor Author

0xTim commented Jan 16, 2025

@Lukasa I've updated the existing public APIs to use the new type to avoid confusing the compiler since the APIs are not usable in their current form. It you'd prefer all the existing APIs to be deprecated and add @_disfavoredOverload instead I can do that

@Lukasa
Copy link
Contributor

Lukasa commented Jan 17, 2025

Thanks Tim. Yeah, can we use overloads and @_disfavoredOverload? I think it would also be enough to deprecate the old APIs: you can easily test that by omitting the disfavored overload keyword and confirming everything still compiles without warnings.

@0xTim
Copy link
Contributor Author

0xTim commented Jan 17, 2025

Updated - I was getting compiler errors due to "ambiguous references..." without the disfavoured overloads so added them in

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've left a few notes in the diff.

@0xTim
Copy link
Contributor Author

0xTim commented Feb 2, 2025

@Lukasa all these should be fixed now

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Ok great, this looks really good. One quick note though, can we add a few very simple tests of these new APIs? Just inserting handlers and confirming they're there is sufficient.

@0xTim
Copy link
Contributor Author

0xTim commented Feb 3, 2025

Ok great, this looks really good. One quick note though, can we add a few very simple tests of these new APIs? Just inserting handlers and confirming they're there is sufficient.

Yes that would be helpful - added!

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Ok, this LGTM. Let's see what CI says.

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Feb 3, 2025
@Lukasa
Copy link
Contributor

Lukasa commented Feb 3, 2025

Looks like the format check wants some cleanups, mind tackling that for me?

@0xTim
Copy link
Contributor Author

0xTim commented Feb 3, 2025

Done

@Lukasa Lukasa enabled auto-merge (squash) February 4, 2025 10:07
@Lukasa Lukasa merged commit ac79370 into apple:main Feb 4, 2025
32 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants