Generate Async Overloads for All Public Functions

[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 using this template 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:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

[curquiza]

I'm ok with this PR, thank you!

Let's wait for the @aronbudinszky feedback before merging.
If we don't get any answer, we will merge it.

[aronbudinszky]

Sorry @Sherlouk somehow missed your comment about tests above - though I kinda agree that parity is theoretically assured, I still think that tests are important because going to async can sometimes cause weird issues and/or there can be some exceptions to how things are implemented exactly.

Also in case there are any test coverage requirements (which probs there should be) this would help with those stats... :)

[aronbudinszky]

@curquiza Just to confirm, this is not a breaking change, so if we choose to add tests in an upcoming PR I'm also happy to merge this in.

[aronbudinszky]

(On a side note as I see there are plans to not allow untested code to be merged.)

[curquiza]

Thanks @aronbudinszky for your reactivity.
About tests you are right, and I would rather we have the tests in the same PR than the addition, if possible 😊

[Sherlouk]

Just acknowledging that I've seen the comments above and will look into writing tests to cover the new asynchronous methods.

[Sherlouk]

Spent a little bit of time looking into the tests, and so wanted to see what folks thought a good balance would be for now:

1. Duplicate all tests so every functionality has an async/await test and a closure based test
2. Replace all closure based tests with async/await tests (under the basis that every async implementation calls the closure based one so in terms of test coverage it's identical and all integrations are touched)
3. Create minified tests to async/await to simply assert a request to the correct API URL path but no assertions on response type (which is covered by existing closure tests).

I'm wary of adding too much duplication and maintenance overhead with this PR, especially until I get to rewriting some of the unit and integration tests to reduce prose.

My opinion would be continuing with number two for now, moving us in the strategic direction of defaulting to async/await, reducing complexity in tests (with use of expectations), and by extension improving performance of tests. It still guarantees a 100% test coverage for public APIs.

[Sherlouk]

I've pushed up a couple examples of my proposal to aide in discussion, the new tests are running about 80% faster locally.

I also believe them to be far more readable, though this will be especially prevalent when I get to the more complex tests with many nested levels of callbacks.

[curquiza]

Sorry for the delay @Sherlouk

I'm ok to go with solution 2

Let me know when you think this is ready to be merged for you!

[Sherlouk]

Thanks Clem, will give @aronbudinszky some time to reply too to ensure we're all aligned and then will get it finished.

[Sherlouk]

bors merge

[Sherlouk]

Going to send this through for now to unlock further work as this is a predecessor for quite a few bits.

If Aron or any other consumer of the application has any opinions on how we can improve this epic then I'd love to hear it - feel free to open a new issue and we can iterate on this together!

[meili-bors]

Build succeeded:

• integration-tests on macos-latest
• integration-tests on ubuntu-20.04
• linter-check