Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Decouple from Guzzle #39

Closed
Xesau opened this issue Oct 27, 2023 · 8 comments
Closed

Feature request: Decouple from Guzzle #39

Xesau opened this issue Oct 27, 2023 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Xesau
Copy link

Xesau commented Oct 27, 2023

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 or symfony/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

@hkulekci
Copy link
Owner

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.

@hkulekci
Copy link
Owner

hkulekci commented Nov 2, 2023

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.

@Xesau
Copy link
Author

Xesau commented Nov 2, 2023

I’ve been a bit busy this week but I think I’ll have time next week to work on this!

@Xesau Xesau changed the title BC Feature request: Decouple from Guzzle Feature request: Decouple from Guzzle Nov 3, 2023
@hkulekci hkulekci added enhancement New feature or request help wanted Extra attention is needed labels Dec 28, 2023
@snapshotpl
Copy link
Contributor

Looking forward for this! @Xesau call me if you need a help

@hkulekci
Copy link
Owner

hkulekci commented Jan 14, 2024

@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.

#47

@hkulekci
Copy link
Owner

hkulekci commented Feb 8, 2024

@Xesau I guess, the pr ready to merge. Please check it.That would be great if you can test it with your case.

@Xesau
Copy link
Author

Xesau commented Feb 9, 2024

Sorry, I ended up using a plain HTTP client instead because that was sufficient for my case.

@hkulekci
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants