Skip to content

Support async custom completion closures #782

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 4 commits into from
Jun 4, 2025

Conversation

rgoldberg
Copy link
Contributor

Support async custom completion closures.

Resolve #555

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@rgoldberg rgoldberg force-pushed the 555-async-custom-completion branch from 9916bb6 to 9a298ff Compare May 19, 2025 23:51
@rgoldberg
Copy link
Contributor Author

rgoldberg commented May 20, 2025

@natecook1000 This PR should be simpler to review than #764, so you might want to get this out of the way first.

Thanks for reviewing / merging everything.

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

Looks good! Can you add a couple tests that verify that the async version is usable?

@rgoldberg
Copy link
Contributor Author

rgoldberg commented May 21, 2025

@natecook1000 Added a multi-shell custom completion test for async custom completion handlers.

Thanks for reviewing.

Copy link
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

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

LGTM!

@swift-ci Please test

@natecook1000
Copy link
Member

@swift-ci Please test

@rgoldberg
Copy link
Contributor Author

@natecook1000 I pushed an attempt to fix the Linux build issues. I didn't get any build or test issues on macOS 15.5, but I don't know how to run the Linux tests myself, or how to make the macOS tests mimic the Linux tests.

I assume that the Linux tests have stricter concurrency checking enabled than the macOS & Windows tests do. Maybe they should be changed to be as strict as the Linux tests. Should I open an issue for that? Does one already exist?

@rgoldberg rgoldberg force-pushed the 555-async-custom-completion branch from d1732af to 020dfd3 Compare May 29, 2025 01:27
@rgoldberg
Copy link
Contributor Author

@natecook1000 Is there any way to get this into 1.5.1? I fixed the issue with the Linux tests, so it should be good to go. Thanks.

@rgoldberg
Copy link
Contributor Author

@natecook1000 Sorry, I just noticed that 1.5.1 only includes the small patch, not all the changes for completions, etc.

I was so worried that these 2 PRs wouldn't get in the new release that I hurried to rebase & comment before completely understanding what was in 1.5.1.

I also wondered why it was 1.5.1 instead of 1.6.0, but now I know. Sorry for the misunderstanding. Looking over the unreleased notes now.

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit ec562e5 into apple:main Jun 4, 2025
28 checks passed
@rgoldberg rgoldberg deleted the 555-async-custom-completion branch June 4, 2025 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permit asynchronous code to be given to the custom completion type
2 participants