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

Allow setting query params for HTTPRequest #1180

Closed
wants to merge 3 commits into from

Conversation

skazhy
Copy link
Contributor

@skazhy skazhy commented Sep 11, 2014

Allow setting queryparams explicitly via query_params in HTTPRequest constructor.

This makes things less error prone, as urlencoding is done automatically, via the already existing httputils.url_concat method.

@skazhy
Copy link
Contributor Author

skazhy commented Sep 11, 2014

Current implementation breaks websocket module. Working on an improved version.

@bdarnell
Copy link
Member

Why wouldn't you just call url_concat directly if you want this?

uri is a poor name for the new property since the difference between it and the existing url property has nothing to do with the difference between URIs and URLs. In general I prefer to avoid properties since they make it less obvious how many times a computation is done - in this case I'd just compute the concatenated string in the constructor.

I haven't tested but it looks like this would incorrectly re-apply the query parameters when following redirects.

@bdarnell
Copy link
Member

bdarnell commented Jun 3, 2018

Closing due to inactivity.

@bdarnell bdarnell closed this Jun 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants