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

add get http.Client interface #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhangyongding
Copy link

Connection adds a new Client () interface for customizing http.Client

@otoolep
Copy link
Member

otoolep commented Nov 6, 2024

While the goal makes sense, this doesn't seem like idiomatic Go. It would be better if this library was modified to take a HTTP client object, instead of creating one on the fly.

I realize that would be a lot more work however.

@zhangyongding
Copy link
Author

Can you check the new pr?

@otoolep
Copy link
Member

otoolep commented Nov 20, 2024

Thanks.

The fundamental issue here however is that the Go code in this library is not in great shape. It doesn't use proper Go patterns, so this change is kinda making it worse (though I understand why).

This library needs a rewrite, but one that doesn't change the API. For example it needs proper NewConnection calls, for example, into which one would pass a HTTP client.

@otoolep
Copy link
Member

otoolep commented Nov 20, 2024

@zhangyongding -- is there something specific you want to control? That you can't do today?

@zhangyongding
Copy link
Author

I hope to open up the rqliteApiCall interface and the parameter settings in the assemblleURL interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants