Closed Xesau closed 5 months ago
Hello @Xesau,
First of all, thank you so much for taking the time to delve into the structure of the project and providing such detailed feedback. And I have the same concerns for Guzzle, and I just tried to keep that part of the code separate. I would be more than happy to review a Pull Request if you are willing to contribute to this enhancement. It sounds like a valuable improvement. We aim to maintain backward compatibility as much as possible, but I believe this change will be helpful for other libraries using this client. So, for this reason, it is okay for me.
If you need any further clarification on the codebase or if there's anything specific you'd like to discuss regarding the implementation, please do not hesitate to ask. I’m looking forward to potentially collaborating on this improvement!
Thanks.
Hey, @Xesau, do you mind working on this, or do you have something you worked on? I'd be excited to test this on the weekend. Please share with me if you have ongoing things.
I’ve been a bit busy this week but I think I’ll have time next week to work on this!
Looking forward for this! @Xesau call me if you need a help
@Xesau and @snapshotpl, I just installed symfony-http in dev and removed Guzzle and its related dependencies. It is not finished yet, but I guess we have pretty good progress. Please check it and review it. I need to think about config again. At this moment, implementing this change will result in a loss of backward compatibility.
@Xesau I guess, the pr ready to merge. Please check it.That would be great if you can test it with your case.
Sorry, I ended up using a plain HTTP client instead because that was sufficient for my case.
Thank you @Xesau. I am happy to hear that you solved the issue with the HTTP client. I just merged the PR and released it as v0.5.3. I hope it helps others. Feel free to use it if you need to. Thanks for your feedbacks as well.
The way the project is structured is a bit odd. It depends on Guzzle, but the code's actual dependency on Guzzle is between the HttpClientInterface abstraction layer. So it's possible to use a different HTTP client (such as
nyholm/http-client
orsymfony/http-client
), but the composer.json forces the user of the library to also load Guzzle, which may be undesirable and create version conflicts.In my opinion, it would make more sense to just use pass a PSR-18
Psr\Http\Client\ClientInterface
along with a Config object into the Qdrant constructor. The Qdrant class could then have a function that takes a PSR-7 RequestInterface, uses the information from Config to set the right path and headers, passes it into the provided HttpClientInterface, and processes the response (the exception handling that is now present in the Guzzle class). You could even extract this function to another class and pass that to the endpoint classes, which makes more sense from the perspective of dependency inversion.I'd be willing to make a PR for this you agree with the proposed direction.
If you have any questions feel free to ask them