Merge lp:~allenap/maas/no-typing-in-zesty into lp:~maas-committers/maas/trunk

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 5614
Proposed branch: lp:~allenap/maas/no-typing-in-zesty
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 74 lines (+29/-10)
2 files modified
src/maastesting/typecheck.py (+21/-0)
src/provisioningserver/boot/__init__.py (+8/-10)
To merge this branch: bzr merge lp:~allenap/maas/no-typing-in-zesty
Reviewer Review Type Date Requested Status
Lee Trager (community) Approve
Review via email: mp+313350@code.launchpad.net

Commit message

Disable type checks when subclass checks do not work with type hints.

Zesty has introduced a subtly new version of `typing` in which Union and its derivatives no longer work with issubclass.

Description of the change

See bug 1650202 for the impetus.

To post a comment you must log in.
Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

I suppose we should wait for the bug to be closed as Invalid/Won't fix before merging this, but otherwise it seems a sensible, if unfortunate, way to work around this.

Revision history for this message
Lee Trager (ltrager) wrote :

I'm fine landing this now so we can start testing Zesty. Please keep an eye on the bug to see if/when we can turn this back on.

review: Approve
Revision history for this message
Mike Pontillo (mpontillo) wrote :

As a follow-on to this branch, we might consider adding a "unit test" for this for the typecheck module that is skipped if typing_is_broken, so that the unit test framework will call out the problem.

(I assume typecheck's unit tests fail horribly on Zesty?)

Revision history for this message
Gavin Panella (allenap) wrote :

> (I assume typecheck's unit tests fail horribly on Zesty?)

It, ahem, has no unit tests.

I wrote it expecting mypy to get better so we could remove it, but mypy isn't functional enough yet (and/or is broken depending on your perspective).

There is a package on PyPI called typeguard [1] that we could use to replace typecheck, but I haven't tried it out. This branch was solely about unblocking MAAS on Zesty, but if we get time we should see if typeguard can be used as a Zesty-compatible replacement for typecheck, and then do the work to switch.

[1] https://pypi.python.org/pypi/typeguard

Revision history for this message
Gavin Panella (allenap) wrote :

Also, I'm feeling a bit disillusioned with the whole type-hinting / gradual-typing thing in Python, which I talked about a bit on IRC yesterday. Python may be better off without the additional complexity.

I like being able to annotate functions as a way to provide better documentation and more readable code. I intended typecheck as one mechanism to help keep that aspect — the documentation aspect — up-to-date.

A secondary effect was to catch coding errors a little earlier, but in practice that hasn't made much of a difference to me, though others may have had a more positive experience.

The typing module has such an un-Pythonic feel, and doesn't really do much on its own, especially if issubclass no longer works. It seems to need mypy to be useful, but mypy seems experimental right now, academic. The typeguard package may be sufficiently compelling to make the use of typing more palatable, but for now we may be better served by instead using the ABCs in collections, collections.abc, and those we define ourselves. ABCs support isinstance and issubclass tests and, even though they're a bit weird, are fairly easy to use and grok in practice.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maastesting/typecheck.py'
2--- src/maastesting/typecheck.py 2016-06-22 17:03:02 +0000
3+++ src/maastesting/typecheck.py 2016-12-15 13:45:33 +0000
4@@ -13,6 +13,17 @@
5 import inspect
6 import typing
7
8+# Some releases of the `typing` module, even within the same major/minor/patch
9+# version of Python, have changed their issubclass behaviour, breaking this
10+# module. Changing a module's public API without bumping Python's version is
11+# not cool. See bug 1650202.
12+try:
13+ issubclass(str, typing.Optional[str])
14+except TypeError:
15+ typing_is_broken = True
16+else:
17+ typing_is_broken = False
18+
19
20 class AnnotationError(TypeError):
21 """An annotation has not be understood."""
22@@ -105,6 +116,16 @@
23 return checked
24
25
26+if typing_is_broken:
27+ def typed(func): # noqa
28+ """Return `func` unchanged.
29+
30+ This release of Python has a "broken" version of `typing` so no type
31+ checks are attempted.
32+ """
33+ return func
34+
35+
36 def get_types_in(hints, func):
37 """Yield type annotations for function arguments."""
38 for name, hint in hints.items():
39
40=== modified file 'src/provisioningserver/boot/__init__.py'
41--- src/provisioningserver/boot/__init__.py 2016-12-07 12:46:14 +0000
42+++ src/provisioningserver/boot/__init__.py 2016-12-15 13:45:33 +0000
43@@ -15,11 +15,7 @@
44 from errno import ENOENT
45 from io import BytesIO
46 import os
47-from typing import (
48- Dict,
49- List,
50- Optional,
51-)
52+from typing import Dict
53
54 from provisioningserver.boot.tftppath import compose_image_path
55 from provisioningserver.events import (
56@@ -279,11 +275,13 @@
57 assert isinstance(self.name, str)
58 assert isinstance(self.bios_boot_method, str)
59 assert isinstance(self.bootloader_path, str)
60- # Union types must be checked with issubclass().
61- assert issubclass(type(self.template_subdir), Optional[str])
62- assert issubclass(type(self.bootloader_arches), List[str])
63- assert issubclass(type(self.bootloader_files), List[str])
64- assert issubclass(type(self.arch_octet), Optional[str])
65+ assert isinstance(self.template_subdir, str) or (
66+ self.template_subdir is None)
67+ assert isinstance(self.bootloader_arches, list) and all(
68+ isinstance(element, str) for element in self.bootloader_arches)
69+ assert isinstance(self.bootloader_files, list) and all(
70+ isinstance(element, str) for element in self.bootloader_files)
71+ assert isinstance(self.arch_octet, str) or self.arch_octet is None
72
73 def get_template_dir(self):
74 """Gets the template directory for the boot method."""