Replace macro generated builder types with unrolled ones

palash25 commented 1 week ago

Fixes: #195

All Submissions:

[ ] Contributions should target the dev branch. Did you create your branch from dev? Apologies, i just saw this, should I try to cherry pick this commit into a new branch checked out from dev and make a new PR?
[ ] Have you followed the guidelines in our Contributing document? I didn't find any Contributing.md or any instructions in the README.md hence the goof up with the branch checkout
[ ] Have you checked to ensure there aren't other open Pull Requests for the same update/change? yes there is one but that seems to be following the wrong approach of handwriting all the unrolled macros

New Feature Submissions:

[ ] Does your submission pass tests?
[ ] Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
[ ] Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

[ ] Have you added an explanation of what your changes do and why you'd like us to include them?
[ ] Have you written new tests for your core changes, as applicable?
[ ] Have you successfully ran tests with your changes locally?

closes https://github.com/qdrant/rust-client/issues/195 /claim https://github.com/qdrant/rust-client/issues/195

palash25 commented 1 week ago

i decided to open this up for visibility. The bulk of the work is done, i used cargo expand to unroll the macro and used scripts to write them to an individual file under a new builders/ directory.

As of now cargo build works but I haven't replaced each and every import of the Builder types, I have only checked with one, so it might take a day to update all the test snippets.

I mistakenly checked out my branch from master instead of dev, i just saw the requirement while making the PR, I will try to see if I can cherry pick my commit into a new branch checked out from dev and replace this PR with a new one. Hopefully it should be done by tomorrow.

I would discourage a review as of now since I have not tested all the Builder types (just one) and this PR is huge. If you still want to test it out, feel free to pull the branch and run cargo build

generall commented 1 week ago

Hey @palash25, thanks for the PR!

One of the requirements of the task is that snippets should not change, even if it is a change of imports. Since we are developing a library, which is publically available and integrates in other products, this requirement is really important, we can't break version compatibility.

Would it be possible to adjust imports so that snippets won't change?

palash25 commented 1 week ago

I will take a look doing away with any import changes. I just didn't want all the builder files to be scattered in the src directory so i created a subdir

generall commented 1 week ago

maybe it would be possible to keep them in one place but re-import

palash25 commented 5 days ago

update: this turned out to be a bit more involved than i anticipated but I was able to get the tests to work for CreateCollectionBuilder without changing import paths in the test snippets. Now i just need to replicate this for all the other types it might take a day or two more. I will then open a new PR against dev branch, this one is just a placeholder for now

palash25 commented 4 days ago

closing in favour of this https://github.com/qdrant/rust-client/pull/199

qdrant / rust-client