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

Use inline type hint syntax #381

Merged
merged 13 commits into from
Aug 26, 2020
Merged

Use inline type hint syntax #381

merged 13 commits into from
Aug 26, 2020

Conversation

wsanchez
Copy link
Member

@wsanchez wsanchez commented Jul 24, 2020

Use inline type hint syntax
Replace Text with str.

Note that _strategies.py is intentionally excluded here because it's moving to hyperlink and keeping that diff smaller seems like a better move.

This is a very large diff, but the vast majority of it is mechanical conversation from comment-based type syntax to inline syntax, which is possible now that we require Python 3.6+. There is some slightly more interesting change in _isession.py due to the fact that the inline syntax causes the interpreter to notice things at runtime that it didn't in the comments; that is noted in review comments.

@wsanchez wsanchez requested a review from a team as a code owner July 24, 2020 11:24
@wsanchez wsanchez self-assigned this Jul 24, 2020
@twm
Copy link
Contributor

twm commented Jul 25, 2020

Looks like flake8 is displeased.

@wsanchez wsanchez marked this pull request as draft July 27, 2020 17:02
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #381 into master will increase coverage by 0.00%.
The diff coverage is 99.76%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   98.36%   98.36%           
=======================================
  Files          46       46           
  Lines        3909     3910    +1     
  Branches      256      256           
=======================================
+ Hits         3845     3846    +1     
  Misses         46       46           
  Partials       18       18           
Impacted Files Coverage Δ
src/klein/_typing.py 84.61% <50.00%> (ø)
src/klein/_app.py 100.00% <100.00%> (ø)
src/klein/_decorators.py 100.00% <100.00%> (ø)
src/klein/_dihttp.py 96.49% <100.00%> (ø)
src/klein/_form.py 99.26% <100.00%> (ø)
src/klein/_headers.py 100.00% <100.00%> (ø)
src/klein/_headers_compat.py 100.00% <100.00%> (ø)
src/klein/_iform.py 100.00% <100.00%> (ø)
src/klein/_imessage.py 100.00% <100.00%> (ø)
src/klein/_interfaces.py 100.00% <100.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ac9ab9...cae4866. Read the comment docs.

@wsanchez wsanchez marked this pull request as ready for review July 27, 2020 18:36
@@ -37,14 +32,96 @@ class TransactionEnded(Exception):
"""


class SessionMechanism(Names):
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved class def up so that it's defined before it is used (as a type) later in this file.

Comment on lines -16 to -19
if TYPE_CHECKING: # pragma: no cover
from ._requirer import RequestLifecycle

IRequestLifecycleT = Union[RequestLifecycle, "IRequestLifecycle"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Getting rid of IRequestLifecycleT for consistency in how we handle internal type hacking currently for zope.interface, see _IRequestLifecycle def below.

Comment on lines +323 to +335
class _IRequestLifecycle(Interface):
"""
Interface for adding hooks to the phases of a request's lifecycle.
"""


if TYPE_CHECKING:
from typing import Union
from ._requirer import RequestLifecycle

IRequestLifecycle = Union[_IRequestLifecycle, RequestLifecycle]
else:
IRequestLifecycle = _IRequestLifecycle
Copy link
Member Author

Choose a reason for hiding this comment

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

When type checking, we use a Union to work around lack of support for zone.interface in Mypy.

Normally we do this in interfaces.py, but we're doing it here (also) due to the fact that we use IRequestLifecycle later in this file and importing from interfaces.py will cause a circular import loop.

All of this should go away when we start using the Zope plug-in for Mypy.

@@ -22,61 +22,61 @@
)

if TYPE_CHECKING: # pragma: no cover
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this diff is sorting the file for later sanity.

We do remove import IRequestLifecycleT as _IRequestLifecycleT and replace that with similar handling of other interfaces for consistency.

@wsanchez wsanchez mentioned this pull request Aug 4, 2020
@wsanchez
Copy link
Member Author

@twisted/kleiniforms I'd really like to knock this PR out since it touches everything and so I'm not going anything else because conflicts.

Since it's all (with one exception) a very robotic moving of type refs from one form to another, and pretty well-validated by mypy, I'd like to push the red button if there's no objection.

The one exception is in _isession.py and noted in a review comment. That was needed because flake8 sees the type hints now and symbols weren't defined in order.

Copy link
Contributor

@rodrigc rodrigc left a comment

Choose a reason for hiding this comment

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

This patch spans many files, but the changes are mostly mechanical changes to use the type annotation syntax for variables and functions/methods.

I raised a few minor questions, but no blockers, so I would suggest to merge this when you can and
resolve other issues later.

src/klein/_headers.py Outdated Show resolved Hide resolved
src/klein/_headers.py Outdated Show resolved Hide resolved
src/klein/_isession.py Outdated Show resolved Hide resolved
isConfidential = Attribute(
"""
A L{bool} indicating whether this session mechanism transmitted over an
encrypted transport, i.e., HTTPS. If C{True}, this means that this
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be L{True}

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

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lot of this. Will save for another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

src/klein/_isession.py Show resolved Hide resolved
@wsanchez
Copy link
Member Author

@rodrigc Thanks! I'll clean up what I can here.

@wsanchez wsanchez merged commit a89861d into master Aug 26, 2020
@wsanchez wsanchez deleted the types-inline branch August 26, 2020 01:20
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