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

Made cookies construct-able from a list of tuples #1211

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

cdeler
Copy link
Member

@cdeler cdeler commented Aug 21, 2020

I changed CookieTypes, added the test as it's said in #1209

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Looks good!

Question: do we allow only lists for other parameters such as headers and data, or do they a more general Sequence of tuples? Just wanted to make sure we're also being consistent there.

@cdeler
Copy link
Member Author

cdeler commented Aug 21, 2020

@florimondmanca
As I see, headers (mentioned in the issue) could be a Sequence

httpx/httpx/_types.py

Lines 40 to 46 in 15c1e42

HeaderTypes = Union[
"Headers",
Dict[str, str],
Dict[bytes, bytes],
Sequence[Tuple[str, str]],
Sequence[Tuple[bytes, bytes]],
]

@florimondmanca
Copy link
Member

Yes, but on the other hand query params accept a List, so hmm. Maybe it's a separate discussion we can have. I do remember that we've had issues with mypy flagging some cases as not acceptable, which required using sequence and mapping instead of list and dict, so maybe that's where these differences come from.

Anyway this PR is consistent with itself - only accept a list of tuples. If we find out that we need to expand to a sequence then let's reconsider at that point? (So far the usage of Dict with cookies doesn't seem to have been a problem.)

@cdeler
Copy link
Member Author

cdeler commented Aug 21, 2020

Now I see that #1209 is related to Sequence (I'm pretty sure that it was List, but may be I need to take a nap).

I see that you have accepted the ticket, but can I change List to Sequence in the PR?

Update
@florimondmanca

I changed typing from List to Sequence, it's a bit confusing for me that I mis-read the original issue.

If commits-after-approval are not allowed, I can revert it back

@florimondmanca
Copy link
Member

@cdeler As discussed on Gitter, could we consider sticking to List instead of Sequence for now?

(We can add Sequence if we need to later, but adding it right now means more API surface that's not necessarily required, which we generally want to keep at the lowest level possible. Makes sense?)

@cdeler
Copy link
Member Author

cdeler commented Aug 22, 2020

@cdeler As discussed on Gitter, could we consider sticking to List instead of Sequence for now?

(We can add Sequence if we need to later, but adding it right now means more API surface that's not necessarily required, which we generally want to keep at the lowest level possible. Makes sense?)

Yes, sure, give me a second

@cdeler
Copy link
Member Author

cdeler commented Aug 22, 2020

@florimondmanca I've reverted the last commit (so the PR contains List for now)

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to merge when you're ready. :)

@tomchristie tomchristie merged commit 6e6ece6 into encode:master Aug 24, 2020
@cdeler cdeler deleted the accept_list_cookies branch September 9, 2020 14:42
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.

3 participants