Add types to some internal modules (`_util`, `_deprecate`, `_ki`) by TeamSpen210 · Pull Request #2719 · python-trio/trio

TeamSpen210 · 2023-07-29T10:34:17Z

Noticed these weren't typed, which will cause issues when typing the decorated functions. Fairly straightforward, with only two functional changes: fixing an invalid use of the deprecation logic which would include the URL twice, and making ki_protection_enabled coerce LOCALS_KEY_KI_PROTECTION_ENABLED to a bool. I think that should be more correct anyway, in case someone assigns a non-bool for some reason.

codecov · 2023-07-29T10:36:26Z

Codecov Report

Merging #2719 (168b5b0) into master (cf1f3c7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2719   +/-   ##
=======================================
  Coverage   98.89%   98.90%           
=======================================
  Files         113      113           
  Lines       16600    16657   +57     
  Branches     3014     3021    +7     
=======================================
+ Hits        16417    16474   +57     
  Misses        126      126           
  Partials       57       57

Files Changed	Coverage Δ
trio/tests.py	`100.00% <ø> (ø)`
trio/__init__.py	`100.00% <100.00%> (ø)`
trio/_core/_ki.py	`100.00% <100.00%> (ø)`
trio/_deprecate.py	`100.00% <100.00%> (ø)`
trio/_tests/test_exports.py	`96.52% <100.00%> (+0.06%)`	⬆️
trio/_tests/test_util.py	`100.00% <100.00%> (ø)`
trio/_util.py	`100.00% <100.00%> (ø)`

jakkdl

Should update pyproject.toml to turn up the strictness on these files. Types otherwise seem fairly straightforward, might try merging it into #2721 and see if that turns up anything.

trio/_util.py

trio/_core/_ki.py

jakkdl · 2023-07-29T11:04:57Z

trio/_util.py


 # Equivalent to the C function raise(), which Python doesn't wrap
-if os.name == "nt":
+elif os.name == "nt":


unrelated to PR, but I've mostly seen this as sys.platform == "win32" in other files. Any reason to prefer one over the other?

Hmm, looking at the docs, os.name is more coarse, and would just give posix I think for Linux, MacOS, etc. But actually type checkers are only required to support platform.

git grep gives os.name 15 times across 11 files, sys.platform 57 times across 29 files. Sounds like something for a dedicated PR to replace instances of the former with the latter.

Looking at SO, seems like os.names coarseness has it's advantages when we don't care about mac/linux distinction if we want users to be able to attempt running trio on weird:tm: systems, as sys.platform can return values outside of win32/linux/darwin.

trio/_util.py

jakkdl

attempted a merge with #2721 and only noticed some minor noteworthy things, as well as Generics without types you'll notice when you up strictness.

trio/__init__.py

trio/_core/_ki.py

trio/_util.py

CoolCat467 · 2023-07-29T22:46:53Z

trio/_util.py

    """

-    def __init__(self, msg):
+    def __init__(self, msg: str):


Add -> None

It's not actually required. Type checkers know that __init__ will always return None, so it's fine to leave it unannotated as long as at least one parameter is annotated to prevent the gradual typing logic from just skipping the method.

Mypy complains if you up the strictness I'm pretty sure

Took a bit of searching, but found this Mypy code which converts -> Any annotations to -> None for __init__. There's also a specific error for not returning None.

I don't have strong opinions either way, but if we want to enforce that it should be enabled - otherwise it's covered by no_untyped_defs only enforcing -> None on __init__ without typed params.

CoolCat467 · 2023-07-30T00:44:38Z

trio/__init__.py

@@ -1,5 +1,6 @@
 """Trio - A friendly Python library for async concurrency and I/O
 """
+from __future__ import annotations  # isort: skip


I don't think you should have to tell isort to skip a __future__ import

You shouldn't no, it's because of the skip directive on the next import. But using split instead behaves more correctly.

trio/_core/_ki.py

trio/_util.py

A5rocks

Looks good

trio/_deprecate.py

trio/_util.py

A5rocks · 2023-07-30T01:13:38Z

trio/_util.py

+# We need ParamSpec to type this "properly", but that requires a runtime typing_extensions import
+# to use as a class base. This is only used at runtime and isn't correct for type checkers anyway,
+# so don't bother.
+class generic_function(t.Generic[RetT]):


I think a runtime import isn't too bad, but now that I think of it I forgot we don't depend on typing_extensions iirc...

yeah, we only depend on it for tests.

jakkdl · 2023-07-30T10:17:25Z

.coveragerc

+  if not t.TYPE_CHECKING:
  if .* or not TYPE_CHECKING:
  if .* or not _t.TYPE_CHECKING:
+  if .* or not t.TYPE_CHECKING:


nit, but could be made cleaner by properly utilizing regexes.

trio/_core/_ki.py

jakkdl · 2023-07-30T10:24:00Z

trio/_util.py

    """

-    def __init__(self, msg):
+    def __init__(self, msg: str):


I don't have strong opinions either way, but if we want to enforce that it should be enabled - otherwise it's covered by no_untyped_defs only enforcing -> None on __init__ without typed params.

trio/_core/_ki.py

jakkdl · 2023-07-30T10:26:11Z

trio/_util.py

+# We need ParamSpec to type this "properly", but that requires a runtime typing_extensions import
+# to use as a class base. This is only used at runtime and isn't correct for type checkers anyway,
+# so don't bother.
+class generic_function(t.Generic[RetT]):


yeah, we only depend on it for tests.

CoolCat467

Looks good to me if everyone agrees about the pyproject.toml changes

TeamSpen210 added 6 commits July 29, 2023 09:32

Add types to the _util module

28f275b

Add types to _deprecate

9f526a2

Fix incorrect use of issue parameter

f23aeeb

Add types to _core._ki

fa16c70

Coerce LOCALS_KEY_KI_PROTECTION_ENABLED to bool

824dad6

Define type for trio.__deprecated_attributes__

bda66ec

TeamSpen210 added the typing Adding static types to trio's interface label Jul 29, 2023

jakkdl requested changes Jul 29, 2023

View reviewed changes

trio/__init__.py Outdated Show resolved Hide resolved

trio/_core/_ki.py Show resolved Hide resolved

trio/_util.py Outdated Show resolved Hide resolved

TeamSpen210 mentioned this pull request Jul 29, 2023

Add type annotations to _depreciate.py #2722

Closed

CoolCat467 reviewed Jul 29, 2023

View reviewed changes

TeamSpen210 added 10 commits July 30, 2023 09:26

Add additional variations of TYPE_CHECKING to coverage

ca1f3fe

Minor tweaks as suggested

097d192

Enable stricter options for these modules

e955f79

This is always an AsyncGenerator

ad12306

Merge remote-tracking branch 'upstream/master' into typed_utils

8b5accd

Update type completeness

c00ffc1

Use annotations future import, dict[] is flagged by mypy in 3.8

acab690

Sort imports and type completeness

0e3939c

Unused import

492db84

Ignore __future__ imports when checking public symbols

22a7473

A5rocks mentioned this pull request Jul 30, 2023

Add typing to _util.py #2726

Closed

CoolCat467 requested changes Jul 30, 2023

View reviewed changes

TeamSpen210 added 3 commits July 30, 2023 11:07

Use isort: split instead of skip, has much more predictable behaviour

342d25e

This typevar is no longer used, so we can import from collections.abc

f4eaa82

Use AsyncGeneratorType instead

0a6f748

A5rocks approved these changes Jul 30, 2023

View reviewed changes

TeamSpen210 added 2 commits July 30, 2023 12:22

Fix quotations

eb72869

Add a test for fixup_module_metadata

8d198ae

TeamSpen210 added 2 commits July 30, 2023 12:27

Use Mapping instead of dict[.., Any]

1584339

Blacken new test

84d586d

TeamSpen210 requested a review from jakkdl July 30, 2023 02:47

Fix coverage problems

168b5b0

jakkdl approved these changes Jul 30, 2023

View reviewed changes

CoolCat467 approved these changes Jul 30, 2023

View reviewed changes

A5rocks merged commit a1f57da into python-trio:master Jul 30, 2023

TeamSpen210 deleted the typed_utils branch July 31, 2023 01:25

jakkdl mentioned this pull request Sep 21, 2023

Typing SocketType, test_socket, test_highlevel_[socket, open_tcp_stream, open_tcp_listeners] #2774

Merged

Uh oh!

Conversation

TeamSpen210 commented Jul 29, 2023

Uh oh!

codecov bot commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jakkdl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakkdl Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakkdl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoolCat467 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jul 29, 2023 •

edited

Loading

jakkdl left a comment •

edited

Loading

jakkdl Jul 29, 2023 •

edited

Loading