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

Type-check test_cookies.py #677

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Type-check test_cookies.py #677

merged 1 commit into from
Dec 23, 2019

Conversation

florimondmanca
Copy link
Member

More tests type-checking, refs #650.

@florimondmanca florimondmanca added the tooling Changes to our CI/CD, tests setup, etc. label Dec 21, 2019
rfc2109=False,
)
cookies.set_cookie(cookie)

client = Client(dispatch=MockDispatch())
client.cookies = cookies
client.cookies = cookies # type: ignore
Copy link
Member Author

@florimondmanca florimondmanca Dec 21, 2019

Choose a reason for hiding this comment

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

mypy doesn't yet support setters that accept arguments of a different type than what is returned by the getter. The official recommandation is to # type: ignore for now. See: python/mypy#3004

Copy link
Member

Choose a reason for hiding this comment

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

Hrm. Well, it is a strange usage. Got me thinking that maybe we just shouldn’t be doing that in the first place...

Instead, we could enforce this style:

client.cookies = httpx.Cookies(...)

That’s possibly less surprising than setting the cookies to a dict, but having it return a Cookies instance.

We would probably want to hard check the type.

Obvs same would apply to the other setters too.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, sure, that would sound good to me. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

-> #678

Copy link
Member

@tomchristie tomchristie 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. Going to suggest that we defer a decision on client.cookies, client.headers, client.params setters at this point in time.

@florimondmanca florimondmanca merged commit 0ee0005 into master Dec 23, 2019
@florimondmanca florimondmanca deleted the typed-cookies-tests branch December 23, 2019 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Changes to our CI/CD, tests setup, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants