Add async overload for search API #410
Conversation
Sources/MeiliSearch/Indexes.swift
Outdated
| @@ -26,7 +26,7 @@ public struct Indexes { | |||
| private let documents: Documents | |||
|
|
|||
| // Search methods | |||
| private let search: Search | |||
| fileprivate let search: Search | |||
There was a problem hiding this comment.
This change is actually not required - private will work just as fine.
There was a problem hiding this comment.
Let me double check this, I thought I had a compiler error when I tested (due to use of an extension) but will double check and revert if possible.
In respect of "I have not actually tested it though" - I'm using this in a production application and is working fine for me 🙂 Though I'll keep an eye on it!
There was a problem hiding this comment.
So I've moved this to internal for the sole reason of improving readability and maintenance. I'm not keen while we're in this split world to have every implementation in one file, I think it'll get a bit much. Internal allows us to break it down into two files (and eventually get rid of one when we only have async). Hope this works for you 🙂
There was a problem hiding this comment.
Yeah that's fair, agreed. (If there is value in hiding these properties internally we could still use protocols for that - though not sure if it is necessary.)
Only final point that I'd make then is that I would not explicitly state internal - given that this is the default anyway and not used on other internal properties.
There was a problem hiding this comment.
Fair point, that's a personal preference but deviates from standard practice for the project. I'll amend in a follow up PR
|
Thank you @aronbudinszky for reviewing and @Sherlouk for the PR Can you confirm there is no breaking for the user? 😊 |
|
This is not a breaking change, no. |
This acts as an example for the conversation being held in issue meilisearch#332. Async/await continues to grow in popularity and all modern codebases are moving towards its usage. This commit demonstrates how we can add support for Swift Concurrency without breaking existing compatibility with older operating systems. For a test I have taken an existing test, copied it, and converted it for async/await. In this instance, removing the XCTestExpectation and simply using an async test available since Xcode 13 back in September 2021.
I believe as we increase this implementation to support more APIs, it will become unwieldly stuck in one file. This commit pulls it out into a separate directory for ease of maintenance.
Sherlouk
force-pushed
the
add-concurrency
branch
from
9b606ed to
338faad
Compare
|
I've updated documentation, tweaked implementation, and opened up the PR. This does only add support for one API (search), but if we're happy then I can either:
Clem, thoughts? My opinion would be #1 or #2 as the async code is very repetitive and so shouldn't be a chore to review (though large) - but might get confusing if mixed in with other features. This'll help to get support quickly to the project. I might do some test refactoring too to enable reuse but validate all behaviour. |
|
Btw retroactively confirming there is no breaking change. :) |
427: Generate Async Overloads for All Public Functions r=Sherlouk a=Sherlouk # Pull Request ## Related issue Fixes #332 This is NOT a breaking change. ## What does this PR do? - Adds async/await overloads to all public interfaces. The contents of these files has been automatically generated using [Sourcery](https://github.com/krzysztofzablocki/Sourcery) using [this template](https://gist.github.com/Sherlouk/fa3e49e24f9be7232c642b945177b05e) I made. It does not include documentation, I feel like this is an okay compromise at this point in time and is something that can be incremented on in the future, potentially when we move over entirely. Async/Await functions sit in Xcode along with the normal closure based methods and so is easy to see the documentation inline anyway. The template has been shared to hopefully make it easier to update when new APIs are added in the future, keeping maintenance down. With the previous PR (#410) I called the underlying implementation directly, this leads to unnecessary duplication with other functions. Instead I call the non-async function guaranteeing consistent functionality, and no unnecessary duplication. I also feel like this avoids the need for excessive and additional test coverage, so long as the build compiles then the implementation is working as expected and tests give us no extra confidence that it's working as expected. We'll keep the one for search to demonstrate. `@aronbudinszky` Are you happy with this approach too? ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: James Sherlock <[email protected]>
Pull Request
Related issue
Relates to #332 (doesn't close as only adds support for one API)
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!