Skip to content

implement type-hints for all (some) our functions #54

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

Merged
merged 5 commits into from
Sep 4, 2017

Conversation

igalic
Copy link
Collaborator

@igalic igalic commented Sep 1, 2017

gradually add types on all functions and variables.
this effort is guided by mypy.

@@ -27,7 +27,7 @@ class DistributionGenerator:
def __init__(self, host, zfs=None, logger=None):
self.logger = libiocage.lib.helpers.init_logger(logger)
self.zfs = libiocage.lib.helpers.init_zfs(zfs)
libiocage.lib.helpers.init_host(self, host)
self.host = libiocage.lib.helpers.init_host(host, self.logger)
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use named arguments - at least for logger and those kind of dependencies. It's much easier to recognize broken interfaces when being explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack, will change

Copy link
Collaborator Author

@igalic igalic left a comment

Choose a reason for hiding this comment

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

some types still not working

@@ -15,7 +17,7 @@ class IocageEvent:
Base class for all other iocage events
"""

HISTORY = []
HISTORY: List[Any] = []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not ideal, but i have idea how to tell it the type of this class

Copy link
Member

Choose a reason for hiding this comment

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

Guess I can explain that: List[libiocage.lib.events.IocageEvent]

The class attribute, not the instance attribute contains a list of all events occurred. This way every event has access to the full history.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

python (or mypy) can't find the type libiocage.lib.events.IocageEvent

Copy link
Member

Choose a reason for hiding this comment

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

Within this file it's just IocageEvent, not the fqdn :trollface:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that's what I thought too, but that doesn't work either.



def get_zfs():
return libzfs.ZFS(history=True, history_prefix="<iocage>")


def init_host(self, host=None):
def init_host(host: libiocage.lib.Host.Host=None, logger:
libiocage.lib.Logger.Logger=None) -> libiocage.lib.Host.Host:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this return type is now incompatible

Copy link
Member

@gronke gronke Sep 1, 2017

Choose a reason for hiding this comment

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

We're returning a HostGenerator. Internally we don't use the wrappers that flatten the generator to a result list (like Host, Jail, etc)

Copy link
Member

Choose a reason for hiding this comment

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

HostGenerator should be the type returned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack, thanks

self.host = self._class_host(logger=logger)
except:
self.host = libiocage.lib.Host.HostGenerator(logger=logger)
return libiocage.lib.Host.HostGenerator(logger=logger)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with this return

Copy link
Member

@gronke gronke left a comment

Choose a reason for hiding this comment

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

I tend to use the full fqdn for typings for better readability. (e.g. typing.List instead of List). Beside that and the few comments, this looks pretty good !

libiocage.lib.helpers.init_host(self, host)
self.logger = libiocage.lib.helpers.init_logger(logger)
self.zfs = libiocage.lib.helpers.init_zfs(zfs)
self.host = libiocage.lib.helpers.init_host(host, self.logger)
Copy link
Member

Choose a reason for hiding this comment

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

Please use named attributes for everything, but the first argument.

self.logger = libiocage.lib.helpers.init_logger(logger)
self.host = libiocage.lib.helpers.init_host(host, self.logger)

# this could be wrong..er.. now?:
Copy link
Member

Choose a reason for hiding this comment

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

What's this comment for? Do you suspect something to be wrong with the following lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so, we init_zfs(), and then we run the equivalent of the get_zfs() function, and if that's meant to be happening consistently, we may wanna just always do that in init_zfs()

@@ -388,7 +388,8 @@ def __init__(self, *args, **kwargs):

class MissingFeature(IocageException, NotImplementedError):

def __init__(self, feature_name: str, plural: bool=False, *args, **kwargs):
def __init__(self, feature_name: str, plural: bool=False,
*args, **kwargs) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Please break every argument into a new line. Like such:

def __init__(
    self
    feature_name: str
    plural: bool=False,
    *args
    **kwargs
) -> None:
    pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yapf doesn't work for python 3.6, and standard indent can't seem to be able to read minds 😛

Copy link
Member

Choose a reason for hiding this comment

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

I'm running this syntax on python3.6. Why should it not work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.



def get_zfs():
return libzfs.ZFS(history=True, history_prefix="<iocage>")


def init_host(self, host=None):
def init_host(host: libiocage.lib.Host.Host=None, logger:
libiocage.lib.Logger.Logger=None) -> libiocage.lib.Host.Host:
Copy link
Member

Choose a reason for hiding this comment

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

HostGenerator should be the type returned.

@igalic igalic force-pushed the type-hints-rev branch 13 times, most recently from 63db46b to 309de59 Compare September 2, 2017 16:14
@@ -36,7 +36,7 @@

class DistributionGenerator:

_class_release = libiocage.lib.Release.ReleaseGenerator
_class_release = 'libiocage.lib.Release.ReleaseGenerator'
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a string. Later on we instantiate this class.

@@ -91,4 +89,4 @@ def processor(self):

class Host(HostGenerator):

class_distribution = libiocage.lib.Distribution.DistributionGenerator
class_distribution = 'libiocage.lib.Distribution.DistributionGenerator'
Copy link
Member

Choose a reason for hiding this comment

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

it should have been libiocage.lib.Distribution.Distribution here. Line 35 should not be deleted as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

n.b.: In class HostGenerator this property was _class_distribution (yay ☭) but here it's without the leading _.

self.datasets = libiocage.lib.Datasets.Datasets(
root=root_dataset,
logger=self.logger,
zfs=self.zfs
)
self.distribution = self._class_distribution(
self.distribution = libiocage.lib.Distribution.DistributionGenerator(
Copy link
Member

Choose a reason for hiding this comment

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

It is important to instantiate from the class attribute _class_distribution. If you use a non-generator class like Host, you don't expect a generator distribution.

Copy link
Member

Choose a reason for hiding this comment

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

@igalic as we discussed offline, this change should be reverted

@@ -79,8 +79,7 @@ def __init__(self,

dict.__init__(self)

libiocage.lib.helpers.init_logger(self, logger)

self.logger = libiocage.lib.helpers.init_logger(logger)
Copy link
Member

Choose a reason for hiding this comment

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

I like to use the output from the helper to store explicitly as self.logger, but we should remain passing self as first attribute. This way we do not need to pass other named arguments explicitly later on.

libiocage.lib.helpers.init_zfs(self, zfs)
libiocage.lib.helpers.init_host(self, host)
self.logger = libiocage.lib.helpers.init_logger(logger)
self.host = libiocage.lib.helpers.init_host(host, logger=self.logger)
Copy link
Member

Choose a reason for hiding this comment

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

Here's one example why we should pass self as first argument to the init_ helpers.

feature_name: str,
plural: bool=False,
*args,
**kwargs) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Please use this style for multi-line method interfaces

def __init__(self,
    feature_name: str,
    plural: bool=False,
    *args,
    **kwargs
) -> None:

@igalic igalic force-pushed the type-hints-rev branch 5 times, most recently from fc1be12 to c02d426 Compare September 4, 2017 08:54
Copy link
Collaborator Author

@igalic igalic left a comment

Choose a reason for hiding this comment

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

not sure this is correct

@@ -91,4 +91,4 @@ def processor(self):

class Host(HostGenerator):

class_distribution = libiocage.lib.Distribution.DistributionGenerator
_class_distribution = libiocage.lib.Distribution.DistributionGenerator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this supposed to be a generator?
is it supposed to be class_distribution and not _class_distribution

Copy link
Member

Choose a reason for hiding this comment

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

As rule of thumb: when the class is not a generator it is not supposed to expose or use generators at all. So, this should be a Distribution, not DistributionGenerator.

Even tho we're using mypy to guide us in this endavour, we don't add it
as dependency, because it's still considered experimental.

But we do allow travis to check our types for us!
use 'forward references' of types that haven't been properly imported yet
Including IocageEvent - which is referenced from from itself!

fix distribu forward ref
this confuses the type-checker ;)
@igalic igalic changed the title implement type-hints for all our functions implement type-hints for all (some) our functions Sep 4, 2017
@gronke
Copy link
Member

gronke commented Sep 4, 2017

@igalic besides that init_* functions should remain having self as first argument, so that we don't need to pass other attributes (like logger, host, etc) on every helpers.init_* call. It's easier to update on inter-dependency changes in future updates as well.

@gronke gronke merged commit 572fac9 into bsdci:master Sep 4, 2017
@igalic igalic deleted the type-hints-rev branch September 4, 2017 14:26
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.

2 participants