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

The definition of LooseCookies is too narrow #4205

Open
serhiy-storchaka opened this issue Oct 17, 2019 · 9 comments · Fixed by #4218
Open

The definition of LooseCookies is too narrow #4205

serhiy-storchaka opened this issue Oct 17, 2019 · 9 comments · Fixed by #4218

Comments

@serhiy-storchaka
Copy link
Contributor

@serhiy-storchaka serhiy-storchaka commented Oct 17, 2019

Seems both CookieJar.update_cookies() and ClientRequest.update_cookies() accept also Iterable[Tuple[str, Union[Morsel, str]] and Mapping[str, Union[Morsel, str]].

The definition of LooseCookies which does not include the above types is not compatible with existing user code.

@asvetlov

This comment has been minimized.

Copy link
Member

@asvetlov asvetlov commented Oct 17, 2019

Agree. Good first-time issue.
Any volunteer?

@punndcoder28

This comment has been minimized.

Copy link

@punndcoder28 punndcoder28 commented Oct 17, 2019

Hey! Can I work on this? What is to be done exactly?

@AtomsForPeace AtomsForPeace mentioned this issue Oct 17, 2019
4 of 5 tasks complete
@helpr helpr bot added pr-available pr-rejected and removed pr-available labels Oct 18, 2019
asvetlov added a commit that referenced this issue Oct 18, 2019
* add more types to LooseCookies #4205

* predefine the Union types for LooseCookies

* add CHANGES fragment
@helpr helpr bot added pr-merged and removed pr-rejected labels Oct 18, 2019
asvetlov added a commit that referenced this issue Oct 18, 2019
* add more types to LooseCookies #4205

* predefine the Union types for LooseCookies

* add CHANGES fragment
(cherry picked from commit e048934)

Co-authored-by: Adam Bannister <adam.p.bannister@gmail.com>
@serhiy-storchaka

This comment has been minimized.

Copy link
Contributor Author

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2019

What about Iterable[Tuple[str, str]] and Mapping[str, str]?.

@asvetlov asvetlov reopened this Oct 18, 2019
@serhiy-storchaka

This comment has been minimized.

Copy link
Contributor Author

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2019

Would be nice to add test cases for update_cookies() with different types of arguments.

@asvetlov

This comment has been minimized.

Copy link
Member

@asvetlov asvetlov commented Oct 18, 2019

@serhiy-storchaka thanks for the note.
@AtomsForPeace would you create a follow-up PR?

asvetlov added a commit that referenced this issue Oct 18, 2019
* add more types to LooseCookies #4205

* predefine the Union types for LooseCookies

* add CHANGES fragment
(cherry picked from commit e048934)

Co-authored-by: Adam Bannister <adam.p.bannister@gmail.com>
@AtomsForPeace

This comment has been minimized.

Copy link
Contributor

@AtomsForPeace AtomsForPeace commented Oct 21, 2019

Sure thing

@AtomsForPeace

This comment has been minimized.

Copy link
Contributor

@AtomsForPeace AtomsForPeace commented Oct 21, 2019

So I added the extra two types mentioned and added a test for CookieJar.update_cookies. Should I add a test for ClientRequest.update_cookies too or is that unnecessary?

@serhiy-storchaka

This comment has been minimized.

Copy link
Contributor Author

@serhiy-storchaka serhiy-storchaka commented Oct 21, 2019

They use similar but independent code, so testing ClientRequest.update_cookies is not redundant.

@AtomsForPeace

This comment has been minimized.

Copy link
Contributor

@AtomsForPeace AtomsForPeace commented Oct 22, 2019

Ok, I'll try find time to add it this evening. Thanks for the feedback

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

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.