Skip to content

Adding params to Client - #372

Merged
sethmlarson merged 8 commits into
encode:masterfrom
tyrelsouza:adding_params_client
Sep 23, 2019
Merged

sethmlarson merged 8 commits into
encode:masterfrom
tyrelsouza:adding_params_client

Conversation

@tyrelsouza

Copy link
Copy Markdown
Contributor

References #368

I models how I did the __getitem__ and __setitem__ after the Headers one, so I chose to normalize the keys to lower.

First time doing Python in a while so figured I'd try to contribute.

@florimondmanca
florimondmanca self-requested a review September 21, 2019 20:46

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Tyrel, thanks for opening this! 👋 I didn't know you were allowed to write Python anymore 😉

Comment thread httpx/models.py Resolved
Comment thread tests/client/test_query_params.py Resolved
Comment thread httpx/client.py Resolved
@tyrelsouza

tyrelsouza commented Sep 21, 2019 •

Copy link
Copy Markdown
Contributor Author

Looking at this comment from @florimondmanca, my __setitem__ doesn't contribute any fixes towards multi-valued queries, wonder if that should be addressed first, or if there's a deeper issue if I keep this __setitem__.

Wonder if the solution is to instead of replace, if a value is there, turn the value into the first item in a list, and then append. (or defaultdict(list) and any rendering always expands upon that?) (thinking aloud I guess..)

@tyrelsouza tyrelsouza changed the title Adding params Client Adding params to Client Sep 22, 2019
@florimondmanca

Copy link
Copy Markdown
Contributor

@tyrelsouza I think #366 will require us to switch the query Paramus data structure to a multidict of some sort. IMO this can (should?) be addressed later — when we make the switch, we’ll come back here and merge multidicts instead of regular dictionaries.

Comment thread httpx/client.py Resolved
Comment thread httpx/client.py Resolved
@florimondmanca

florimondmanca commented Sep 22, 2019 •

Copy link
Copy Markdown
Contributor

Forget about what I said earlier about multidicts — that's obviously already what we're using.

I played around with the bug from #366 and it turns out it can be fixed independently from this. I'll file a fix in a sec. :-) Edit : filed #374

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One extra thing we can do is update the developer interface to show that Client now accepts and exposes params too. Would you like to take that on?

@tyrelsouza

Copy link
Copy Markdown
Contributor Author

Sure! I can get to that in two hours. If you don't want to have to wait that long I won't be hurt if I don't do it!

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tyrelsouza! Merging this now :)

@sethmlarson
sethmlarson merged commit 5dc646f into encode:master Sep 23, 2019
@tyrelsouza
tyrelsouza deleted the adding_params_client branch September 23, 2019 14:22
Kludex referenced this pull request in pydantic/httpx2 May 11, 2026
Bumps [black](https://github.com/psf/black) from 21.5b2 to 21.6b0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](https://github.com/psf/black/commits)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tom Christie <tom@tomchristie.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants