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

Url argument in ws and http client. #17

Open
CaedenPH opened this issue Sep 19, 2022 · 6 comments
Open

Url argument in ws and http client. #17

CaedenPH opened this issue Sep 19, 2022 · 6 comments

Comments

@CaedenPH
Copy link
Contributor

Personally, I don't like just passing in the url like py client = HTTPClient("http://localhost:8000", namespace="test", database="test", username="root", password="root") because it is a bit confusing and inefficient. For example, if the url localhost is passed the requests made are actually significantly slower, speaking from past experience.
This is due to localhost being an extra local dns conf lookup.

To prevent this, perhaps there could be a config class for url and port. This way both https and http can be tried to ensure the correct protocol is used. This helps because if a user doesn't understand the difference in the protocols they won't receive raw errors.
In addition, reserved ports can be checked, localhost can be changed to 127.0.0.1 and protocols can be checked.

@tsunyoku
Copy link
Contributor

i don't really see the point in this. localhost being slower has nothing to do with the lib, and some people may be running servers on a domain in the cloud without needing to specify a port - this will get confusing. i think the current system is fine and i do not see it changing, it is the same in the other libraries too.

also, would like to say that localhost != 127.0.0.1 so i really do not like the idea of changing it for "performance" or whatever other reason you may have for it

@tobiemh
Copy link
Member

tobiemh commented Sep 19, 2022

The only issue I see is that localhost doesn't always map to 127.0.0.1, but depends on the user configuration. Also if the user does want localhost, it probably shouldn't be opinionated on whether that should be changed to something else.

I think this is better as a note/warning in the documentation.

@EnokiUN
Copy link

EnokiUN commented Sep 19, 2022

I don't think most libraries out there try to protect the users from themselves that much, if anything IMO you should be atleast that competent before trying to use something as complex as an asynchronous OOP based database wrapper (it might not sound like much for experienced people but boy hanging around discord python servers has taught me a lot).

Also adding a separate class for it is just adding extra unnecessary complexity to something that's extremely simple and is as I've mentioned before borderline work for any sort of library which relies on a service located at some domain.

@CaedenPH
Copy link
Contributor Author

The only issue I see is that localhost doesn't always map to 127.0.0.1, but depends on the user configuration. Also if the user does want localhost, it probably shouldn't be opinionated on whether that should be changed to something else.

I think this is better as a note/warning in the documentation.

True, perhaps this should be implemented as a note/warning in the documentation

@FrostyTheSouthernSnowman
Copy link
Contributor

Why don't we just let the user set the IP to 127.0.0.1 if he/she prefers? We could add a little note in the docs saying that 127.0.0.1 would more performant than localhost.

@tsunyoku
Copy link
Contributor

Why don't we just let the user set the IP to 127.0.0.1 if he/she prefers? We could add a little note in the docs saying that 127.0.0.1 would more performant than localhost.

Yeah, that's the plan!

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

No branches or pull requests

5 participants