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

Add Optional for types on expires/expires_days #2678

Merged
merged 5 commits into from
Jun 17, 2019

Conversation

robinro
Copy link
Contributor

@robinro robinro commented Jun 11, 2019

The runtime already allows and deal with None values here. Adding Optional improves the type checking and (since this is public API) allows users to run their type checker with strict-optional.

The runtime already allows and deal with None values here. Adding
Optional improves the type checking and (since this is public API)
allows users to run their type checker with strict-optional.
@bdarnell
Copy link
Member

Thanks!

Tornado is compatible with strict-optional mode, which has been the default in mypy since version 0.600 (Tornado currently uses mypy 0.700). The issue here is about implicit-optional mode, which is on by default, but that default is intended to change in the indefinite future (python/peps#689, python/typing#275).

I think it's a good idea to switch to no-implicit-optional mode, but we should go all the way: Add no_implicit_optional = True to our setup.cfg, and fix up every instance where we currently use = None without Optional.

@robinro
Copy link
Contributor Author

robinro commented Jun 14, 2019

Thanks @bdarnell for having a look and the clarification regarding strict/implicit optional.

I'll have a go at enabling no_implicit_optional and fixing the related issues.

@robinro robinro force-pushed the typing-optional-set_secure_cookie branch from e5aa680 to cd6cd9c Compare June 14, 2019 07:13
@robinro
Copy link
Contributor Author

robinro commented Jun 14, 2019

@bdarnell I added no_implicit_optional and made the necessary changes to annotations. Please have a look and let me know what you think.

There is currently still one failing test: https://travis-ci.org/tornadoweb/tornado/jobs/545617971
It seems Python 3.5.2 doesn't like Optional[Type["AsyncHTTPClient"]] (also tried Union[Type[".."], None]. Other python version are fine, though. Do you have an idea what's the issue there? I saw some comments about 3.5.2 vs. other 3.5 patchlevels.

@ploxiln
Copy link
Contributor

ploxiln commented Jun 14, 2019

ubuntu-16.04 comes with python-3.5.2, so that's why tornado is attempting to retain compatibility. Unfortunately, there were some significant bugs fixed in later python 3.5 point releases.
#2605
https://stackoverflow.com/questions/42942867/optionaltypefoo-raises-typeerror-in-python-3-5-2

@ploxiln
Copy link
Contributor

ploxiln commented Jun 14, 2019

... so I think one solution is to stringify the whole "Optional[Type[AsyncHTTPClient]]" to defer any parsing to mypy (and mypy checks are not run on python-3.5.2)

@robinro robinro force-pushed the typing-optional-set_secure_cookie branch from 9eaebf7 to 8a55930 Compare June 16, 2019 14:22
@robinro robinro force-pushed the typing-optional-set_secure_cookie branch from 8a55930 to 4beb228 Compare June 16, 2019 14:31
@robinro
Copy link
Contributor Author

robinro commented Jun 16, 2019

Thanks @ploxiln for the hint. The CI is green now. Please let me know if there is anything else to change here.

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Thanks!

@bdarnell bdarnell merged commit c50aed0 into tornadoweb:master Jun 17, 2019
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