Skip to content

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 added 3 commits May 28, 2025 23:45
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
Signed-off-by: Ross Goldberg <484615+rgoldberg@users.noreply.github.com>
@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

2 participants