-
Notifications
You must be signed in to change notification settings - Fork 11
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
Strict optional #88
Strict optional #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of the code here is a bit confusing and will need proper review!
_devfs: 'libiocage.lib.DevfsRules.DevfsRules' = None | ||
releases_dataset: libzfs.ZFSDataset = None | ||
_devfs: 'libiocage.lib.DevfsRules.DevfsRules' | ||
releases_dataset: libzfs.ZFSDataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we don't initialize a class variable?
|
||
def get_zfs( | ||
logger: 'libiocage.lib.Logger.Logger'=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that in ZFS' case, logger
is not used.
@@ -49,4 +42,4 @@ def get_pool(self, name: str) -> libzfs.ZFSPool: | |||
for pool in self.pools: | |||
if pool.name == pool_name: | |||
return pool | |||
return None | |||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the best we can do???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can return errors.ZFSPoolUnavailable
@@ -44,16 +44,14 @@ def init_zfs( | |||
|
|||
if (zfs is not None) and isinstance(zfs, libiocage.lib.ZFS.ZFS): | |||
object.__setattr__(self, 'zfs', zfs) | |||
return zfs | |||
else: | |||
new_zfs = libiocage.lib.ZFS.get_zfs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assign a new variable
return zfs | ||
else: | ||
new_zfs = libiocage.lib.ZFS.get_zfs() | ||
object.__setattr__(self, 'zfs', new_zfs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set the zfs
attribute in both branches.
return zfs | ||
else: | ||
new_zfs = libiocage.lib.ZFS.get_zfs() | ||
object.__setattr__(self, 'zfs', new_zfs) | ||
|
||
try: | ||
return self.zfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to access the zfs
attribute set above.
zfs = libiocage.lib.ZFS.get_zfs(logger=self.logger) | ||
object.__setattr__(self, 'zfs', zfs) | ||
return zfs | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise if it fails
@@ -165,7 +163,7 @@ def validate_name(name: str) -> bool: | |||
return bool(_validate_name.fullmatch(name)) | |||
|
|||
|
|||
def parse_none(data) -> str: | |||
def parse_none(data: typing.Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just telling the truth here;)
25df79d
to
ba3d223
Compare
this allows mypy to follow the existing variables. Refactoring this code lead me to believe that the correct behaviour should be to raise an exception in case of error.
this could be more "elegant", but for now it's "correct"
# Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # On branch strict-optional # Changes to be committed: # modified: libiocage/lib/Host.py #
by ensuring that it stays a dict, mypy has an easier time following it
# Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # On branch strict-optional # Your branch is ahead of 'origin/strict-optional' by 2 commits. # (use "git push" to publish your local commits) # # Changes to be committed: # modified: libiocage/lib/Resource.py #
d052375
to
449557d
Compare
libiocage/lib/Host.py
Outdated
@@ -63,7 +63,7 @@ def __init__( | |||
host=self, | |||
logger=self.logger | |||
) | |||
self._defaults = defaults | |||
self._defaults = defaults if defaults is not None else {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got your point! This should achieve the same without setting _defaults to a property other than None or DefaultResource.
class HostGenerator:
_class_distribution = libiocage.lib.Distribution.DistributionGenerator
_devfs: 'libiocage.lib.DevfsRules.DevfsRules'
_defaults: 'libiocage.lib.Resource.DefaultResource'
releases_dataset: libzfs.ZFSDataset
def __init__(
self,
root_dataset: libzfs.ZFSDataset=None,
defaults: 'libiocage.lib.Resource.DefaultResource'=None,
zfs: 'libiocage.lib.ZFS.ZFS'=None,
logger: 'libiocage.lib.Logger.Logger'=None
) -> None:
self.logger = libiocage.lib.helpers.init_logger(self, logger)
self.zfs = libiocage.lib.helpers.init_zfs(self, zfs)
self.datasets = libiocage.lib.Datasets.Datasets(
root=root_dataset,
logger=self.logger,
zfs=self.zfs
)
self.distribution = self._class_distribution(
host=self,
logger=self.logger
)
if defaults is not None:
self._defaults = defaults
|
||
def __init__( | ||
self, | ||
file: str=None, | ||
file: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file
is explicitly allowed to be None. There are situations where the file is unknown. For example when a new jail gets created before name assignment.
Your changes have been merged to master! |
This PR is an attempt to correct type warnings issued by
mypy --strict-optional
This catches a lot more issues where a variable was previously assigned
None
, or a return type should beOptional
Some of the following code is questionable and needs discussion & review.