implement type-hints for all our functions #42

igalic · 2017-08-28T14:21:57Z

as a first step, we'll ad mypy static analyzer to check those.
This should give us extra (offline) guarantees about those types.

igalic · 2017-08-28T14:22:44Z

this first attempt at #39, is currently failing to bootstrap:

libiocage/cli/__init__.py:38: error: Module has no attribute "_verify_python3_env"
libiocage/cli/__init__.py:102: error: Unexpected keyword argument "message" for "version_option"
/home/igalic/.pyenv/versions/3.6.1/lib/mypy/typeshed/third_party/2and3/click/decorators.pyi:174: note: "version_option" defined here
 ǃ  ~/s/t/libiocage  ╍╍ type-hints 

gronke · 2017-08-28T19:01:07Z

Do we need to add an entry to requirements.txt?

igalic · 2017-08-28T19:16:48Z

new set of errors:

libiocage/lib/helpers.py:52: error: Invalid tuple literal type
libiocage/lib/Jail.py:67: error: Name 'libzfs' is not defined
libiocage/lib/Jail.py:68: error: Invalid type "libiocage.lib.Host"
libiocage/lib/Jail.py:69: error: Invalid type "libiocage.lib.Logger"
libiocage/cli/__init__.py:38: error: Module has no attribute "_verify_python3_env"
libiocage/cli/__init__.py:102: error: Unexpected keyword argument "message" for "version_option"
/home/igalic/.pyenv/versions/3.6.1/lib/mypy/typeshed/third_party/2and3/click/decorators.pyi:174: note: "version_option" defined here

i am working speculatively here, so your help would be greatly appreciated!

igalic · 2017-08-28T19:23:18Z

@gronke: Do we need to add an entry to requirements.txt?

i don't think so: it's not a run-time requirement, only for test, hence an entry in test_requires(), but not in requirements.txt

skarekrow · 2017-08-29T17:17:55Z

libiocage/lib/Jail.py

@@ -60,19 +62,24 @@ class Jail:
            release (stored in `zpool/iocage/base/<RELEASE>)
    """

-    def __init__(self, data={}, zfs=None, host=None, logger=None, new=False):
+    def __init__(self,
+                 data: Union[str, dict]={},


Why is this a union? It is never a string, but always a dict.

because that's exactly what the pydoc comment below says

lol, coffee. You're right, sorry!

yeah, but, we should still figure out if we want to simplify that

as i have done with the execs that would've allowed Union[str, List[str]]

igalic

how to check fully.qualified.class.names?

igalic · 2017-08-30T13:23:30Z

libiocage/lib/Jail.py

-    def __init__(self, data={}, zfs=None, host=None, logger=None, new=False):
+    def __init__(self,
+                 data: Union[str, dict]={},
+                 zfs: Optional[libzfs.ZFS]=None,


i can't verify this on linux ☹

libiocage/lib/Jail.py:67: error: Name 'libzfs' is not defined

which means we won't be able to verify this on Travis…

Indeed that's a problem. Can we provide a Typing file for libzfs?

igalic · 2017-08-30T13:24:02Z

libiocage/lib/Jail.py

+                 data: Union[str, dict]={},
+                 zfs: Optional[libzfs.ZFS]=None,
+                 host: Optional[libiocage.lib.Host]=None,
+                 logger: Optional[libiocage.lib.Logger]=None,


for these two, i'm getting

libiocage/lib/Jail.py:68: error: Invalid type "libiocage.lib.Host" libiocage/lib/Jail.py:69: error: Invalid type "libiocage.lib.Logger"

new data! this is a module, not a type

how do i access the actual type, or the class?

libiocage.lib.Host.Host 🌮

Python interprets the file itself as module. Within that module there's the Host class.

but that's uglt 😅

Yeah, I know! The other option is to put everything in one file. Let's quickly forget that.

Yet another solution is to mess with sys.path, but @william-gr taught me this is a no-go for libraries and bad practice everywhere else.

I think lib/__init__.py is an appropriate solution: The public exposed classes are available as libiocage.Jail, libiocage.Release, etc when importing the directory (for development) or importing globally installed libiocage.

igalic

how to handle multiple return values?

igalic · 2017-08-30T13:27:29Z

libiocage/lib/helpers.py

-    if isinstance(command, str):
-        command = [command]
+def exec(command, logger=None, ignore_error=False) -> (
+        subprocess.Popen, str, str,):


this is… wrong, apparently:

libiocage/lib/helpers.py:52: error: Invalid tuple literal type

igalic · 2017-08-31T13:09:44Z

UPDATE!!
after generating types interfaces for the classes missing them (see: truenas/py-libzfs#4 , vstakhov/libucl#168 & foutaise/texttable#22 ), i now have the following set of errors:

libiocage/lib/helpers.py:52: error: Invalid tuple literal type
libiocage/lib/Jail.py:138: error: Incompatible types in assignment (expression has type "RCConf", variable has type None)
libiocage/lib/Jail.py:161: error: Incompatible types in assignment (expression has type Type[NullFSBasejailStorage], variable has type Type[ZFSBasejailStorage])
libiocage/lib/Jail.py:538: error: Argument 1 to "append" of "list" has incompatible type "Network"; expected "str"
libiocage/lib/Jail.py:542: error: "str" has no attribute "teardown"
libiocage/lib/Jail.py:730: error: Value of type None is not indexable
libiocage/lib/Jail.py:736: error: Value of type None is not indexable
libiocage/lib/Jail.py:782: error: Incompatible types in assignment (expression has type "str", variable has type None)
libiocage/lib/Jail.py:796: error: "str" has no attribute "mountpoint"
libiocage/lib/Jail.py:852: error: Type[Dict[Any, Any]] has no attribute "__dir__"
libiocage/lib/Distribution.py:110: error: Need type annotation for variable
libiocage/lib/Distribution.py:115: error: Incompatible types in assignment (expression has type List[str], variable has type "str")
libiocage/lib/Distribution.py:128: error: "Distribution" has no attribute "host"
libiocage/cli/__init__.py:38: error: Module has no attribute "_verify_python3_env"
libiocage/cli/__init__.py:103: error: Unexpected keyword argument "message" for "version_option"
/usr/local/lib/mypy/typeshed/third_party/2and3/click/decorators.pyi:174: note: "version_option" defined here

igalic

our different Basejail Storage thingies are all… wrong…ish.

igalic · 2017-08-31T14:21:49Z

libiocage/lib/Jail.py

+            libiocage.lib.NullFSBasejailStorage.NullFSBasejailStorage,
+            libiocage.lib.ZFSBasejailStorage.ZFSBasejailStorage,
+            None,
+        ] = None


I have no idea what i'm doing

igalic · 2017-08-31T14:22:07Z

libiocage/lib/Jail.py

+        ] = None
+
+        if self.config["basejail_type"] == "standalone":
+            backend = libiocage.lib.StandaloneJailStorage.StandaloneJailStorage


libiocage/lib/Jail.py:163: error: Incompatible types in assignment (expression has type Type[StandaloneJailStorage], variable has type "Union[StandaloneJailStorage, NullFSBasejailStorage, ZFSBasejailStorage, None]")

Why is it type Type[StandaloneJailStorage], but not type "Type[Union[StandaloneJailStorage, ...."

igalic · 2017-08-31T14:22:26Z

libiocage/lib/Jail.py

+        ] = None
+
+        if self.config["basejail_type"] == "standalone":
+            backend = libiocage.lib.StandaloneJailStorage.StandaloneJailStorage

        if self.config["basejail_type"] == "zfs":
            backend = libiocage.lib.ZFSBasejailStorage.ZFSBasejailStorage


libiocage/lib/Jail.py:166: error: Incompatible types in assignment (expression has type Type[ZFSBasejailStorage], variable has type "Union[StandaloneJailStorage, NullFSBasejailStorage, ZFSBasejailStorage, None]")

for line 169, we get:

libiocage/lib/Jail.py:169: error: Incompatible types in assignment (expression has type Type[NullFSBasejailStorage], variable has type "Union[StandaloneJailStorage, NullFSBasejailStorage, ZFSBasejailStorage, None]")

and finally, for line 172, we get:

libiocage/lib/Jail.py:172: error: Too many arguments for "apply"

gronke · 2017-08-31T23:53:35Z

.travis/flake8.sh

@@ -2,7 +2,7 @@
 # Run pep8 on all .py files in all subfolders

 tmpafter=$(mktemp)
-find ./libiocage/ -name \*.py -exec flake8 --ignore=E203,W391 {} + | grep -v "./libiocage/__init__.py" > ${tmpafter}
+find ./libiocage/ -name \*.py -exec flake8 --ignore=E203,E241,W391 {} + | grep -v "./libiocage/__init__.py" > ${tmpafter}


What is E241 good for? The documentation says E241 (*) | multiple spaces after ‘,’, but I can't spot why we need to ignore this rule.

I agree. I added the ignore for spaces after : so we shouldn't need to add another.

without this,

./libiocage/lib/Jail.py:72:23: E241 multiple spaces after ':' ./libiocage/lib/Jail.py:73:22: E241 multiple spaces after ':' ./libiocage/lib/Jail.py:74:23: E241 multiple spaces after ':' ./libiocage/lib/Jail.py:76:22: E241 multiple spaces after ':'

i've basically just followed @skarekrow's lead of how to indent… structures.

as a first step, we'll ad mypy static analyzer to check those. This should give us extra (offline) guarantees about those types.

in the process i also found a few instances where we were mutating input parameters — unnecessarily so. n.b.: libiocage.lib.Foo is the module, libiocage.lib.Foo.Foo is the actual class that we want

explicitly, with Tuple[foo, bar], not implicitly with (foo, bar,)

but not always right ;)

and not a string..

this leads to some circular, or otherwise impossible imports.

igalic · 2017-09-01T10:48:55Z

i'd like to close this pr.
it was a good learning experience, but it's a complete mess, especially after rebasing #37
i've opened #54, where i'll try to do this over, and, more gradually…

igalic force-pushed the type-hints branch from 36d7a60 to 45f2612 Compare August 28, 2017 19:14

gronke added the work in progress label Aug 28, 2017

skarekrow reviewed Aug 29, 2017

View reviewed changes

igalic mentioned this pull request Aug 29, 2017

Use type-hints for all function arguments & return values #39

Closed

gronke added this to the Stability and Performance milestone Aug 29, 2017

igalic commented Aug 30, 2017

View reviewed changes

igalic force-pushed the type-hints branch 3 times, most recently from 796b72e to 680a198 Compare August 31, 2017 08:09

igalic commented Aug 31, 2017

View reviewed changes

igalic force-pushed the type-hints branch from d37fb25 to 1723529 Compare August 31, 2017 20:03

gronke reviewed Aug 31, 2017

View reviewed changes

igalic added 12 commits September 1, 2017 10:31

implement type-hints for all our functions

dcac8b8

as a first step, we'll ad mypy static analyzer to check those. This should give us extra (offline) guarantees about those types.

Jail: start adding type hints

73983e0

Jail: apparently no one cares if stop() is successful

e718786

Jail: type-hint execs

e33c1dd

in the process i also found a few instances where we were mutating input parameters — unnecessarily so. n.b.: libiocage.lib.Foo is the module, libiocage.lib.Foo.Foo is the actual class that we want

Jail: add return values where unambigous

a72c2c7

typles need to be specified as such

e2f7855

explicitly, with Tuple[foo, bar], not implicitly with (foo, bar,)

type-check Jail#start()

e7f7550

correcting a few mistakes: str is always a good guess

f7d78ff

but not always right ;)

x: Foo=None == x: Optional[Foo]=None

a8dbb5f

fix eol_check types

ed716fc

simplify Jail types around dict

8b784f9

dataset is a libzfs.ZFSDataset!

f99b9a5

and not a string..

igalic added 3 commits September 1, 2017 10:46

ignore E241: multiple space after ":"

f59c676

do not type-check RCCconf for now

541fca4

this leads to some circular, or otherwise impossible imports.

fix flake8: remove Dict again

8377a72

igalic force-pushed the type-hints branch from 4d1195d to 8377a72 Compare September 1, 2017 08:46

igalic closed this Sep 1, 2017

igalic deleted the type-hints branch September 3, 2017 19:19

igalic removed the work in progress label Sep 3, 2017

implement type-hints for all our functions #42

implement type-hints for all our functions #42

Uh oh!

Conversation

igalic commented Aug 28, 2017

Uh oh!

igalic commented Aug 28, 2017

Uh oh!

gronke commented Aug 28, 2017

Uh oh!

igalic commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igalic commented Aug 28, 2017

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!

igalic left a comment

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!

gronke Aug 30, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igalic commented Aug 31, 2017

Uh oh!

igalic left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igalic commented Sep 1, 2017

Uh oh!

Uh oh!

igalic commented Aug 28, 2017 •

edited

Loading

gronke Aug 30, 2017 •

edited

Loading