meilisearch / meilisearch-swift

Swift client for the Meilisearch API

https://www.meilisearch.com

MIT License

93 stars 26 forks source link

Add async overload for search API #410

Closed Sherlouk closed 1 year ago

Sherlouk commented 1 year ago

Pull Request

⚠️ I've marked this as draft as it current works to provide an example of how we could add async/await support to the library. I am more than happy to expand this to all currently supported APIs.

Related issue

Relates to #332 (doesn't close as only adds support for one API)

What does this PR do?

Adds an async/await override to an existing search API providing users with more choice.

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!

curquiza commented 1 year ago

Thank you @aronbudinszky for reviewing and @Sherlouk for the PR

Can you confirm there is no breaking for the user? 😊 Is this change worth adding some documentation to the README? Or is ti really "usage transparent"?

Sherlouk commented 1 year ago

This is not a breaking change, no. I do think it's worth updating documentation with a quick example, but it is also obvious when using.

Sherlouk commented 1 year ago

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:

Merge this PR, and then open a new one with support for all other APIs
Keep this PR open, until I've added support for all other APIs
Merge this PR, but open support for each other API one PR at a time (as I work on them with other features).

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.

curquiza commented 1 year ago

Let's go with #1, not with #3 indeed 😄

meili-bors[bot] commented 1 year ago

Build succeeded:

aronbudinszky commented 1 year ago

Btw retroactively confirming there is no breaking change. :)