flake8: check for mutable function parameters #107

igalic · 2017-09-17T14:31:14Z

install flake8-mutable (and, just to be sure, flake8-builtins;) to check
for these things, as reported in #38.

igalic · 2017-09-17T14:48:44Z

i think we can do better…

install flake8-mutable (and, just to be sure, flake8-builtins;) to check for these things, as reported in #38.

given the amount of changes we've recently made, it makes no sense to copare against the arbitrary 0.0.0 tag we made, just for this script. Instead we install flake8-mypy, and replace our test with a single call to flake8.

(type in this case;) while we're at it, also change the the call-site to named params

i don't know what input is, but flake8-builtin says to not shadow it. okay.

we provide missing types for py-libzfs, ucl, texttable, etc.

when the config doesn't exist

igalic · 2017-09-19T20:03:48Z

.travis/mypy-stubs/libzfs.pyi

@@ -54,21 +54,21 @@ class ZFS:
    datasets = ... # type: Any
    errno = ... # type: Any
    errstr = ... # type: Any
-    pools = ... # type: Any
+    pools: List[ZFSPool] = ...


This whole thing is amazing
thank you very much for the effort of typing this package! 💜

igalic · 2017-09-19T20:08:05Z

iocage/cli/set.py

@@ -66,15 +71,15 @@ def cli(ctx, props, jail):

    updated_jail_count = 0

-    for jail in ioc_jails:
+    for ioc_jail in ioc_jails:  # type: iocage.lib.Jail.JailGenerator


how the heck does that need a hint?? that feels like a bug in mypy

You can try to remove. Being explicit in other places should have fixed it anyway!

you're right
works without it, too.

igalic

some soft review, would love to read your explanation for some of the refactorings

would've loved to read it in the commit message even more ;)

igalic · 2017-09-19T20:08:35Z

iocage/cli/set.py

@@ -107,6 +112,7 @@ def set_properties(
                del target.config[key]
                updated_properties.add(key)
            except:
+                raise
                pass


this pass can probz be removed.

igalic · 2017-09-19T20:09:57Z

iocage/lib/Config/Jail/Defaults.py


    user_properties: set = set()

-    data: dict = {
+    def __init__(self, defaults: dict={}) -> None:


doesn't this make defaults mutable?

not according to flake8-mutable.

igalic · 2017-09-19T20:14:06Z

iocage/lib/Config/Jail/Defaults.py

@@ -26,11 +26,38 @@
 import iocage.lib.Config.Jail.BaseConfig


-class JailConfigDefaults(iocage.lib.Config.Jail.BaseConfig.BaseConfig):
+class DefaultsUserData(dict):


can you briefly explain what happened.

JailConfigDefaults should return all default properties as data, but only the user overrides as user_data, so that only manual overrides are saved to defaults.json. It was not sufficient to provide a reduced copy of self.data as user_data because some properties were references. DefaultsUserData goes one level deeper and intercepts setting individual attributes, so that we no longer need to pass the reduced copy of self.data.

igalic · 2017-09-19T20:21:21Z

iocage/lib/Datasets.py

-            pool.create(name, {})
-            dataset = self.zfs.get_dataset(name)
+            target_pool.create(dataset_name, {})
+            dataset = self.zfs.get_dataset(dataset_name)


igalic · 2017-09-19T20:25:07Z

iocage/lib/Jail.py

+            **resource_args
+        )
+
+        if not new and (("id" not in data) or (data["id"] is None)):


this looks like a candidate for encapsulation ;)

It's below 80 characters!!!1 😼

yea, but, the logic is too much for my tired brain 😩

igalic · 2017-09-19T20:26:19Z

iocage/lib/LaunchableResource.py

-    def dataset(self, value: libzfs.ZFSDataset):
-        self._set_dataset(value)
-
-    @property


what happened here?

It's implemented by the parent class. This overrides are no longer needed.

gronke previously approved these changes Sep 17, 2017

View reviewed changes

igalic dismissed gronke’s stale review via 29d6db8 September 17, 2017 15:01

igalic force-pushed the flake8/more-tests branch from 525108e to 721ca83 Compare September 17, 2017 15:22

gronke force-pushed the flake8/more-tests branch from 2313bae to 07e1fb9 Compare September 17, 2017 18:40

igalic force-pushed the flake8/more-tests branch from 07e1fb9 to c895b1b Compare September 17, 2017 21:29

igalic and others added 19 commits September 19, 2017 19:43

flake8: check for mutable function parameters

57512a7

install flake8-mutable (and, just to be sure, flake8-builtins;) to check for these things, as reported in #38.

flake8: abandon .travis/flake8.sh

141e0e1

given the amount of changes we've recently made, it makes no sense to copare against the arbitrary 0.0.0 tag we made, just for this script. Instead we install flake8-mypy, and replace our test with a single call to flake8.

flake8: fix flake8 issues in our base code

d12f1d5

flake8: new_line's params shadowing a builtin

d5a3bba

(type in this case;) while we're at it, also change the the call-site to named params

refactor Dataset.py

c01c04d

flake8/M511 don't mutate Network#__init__ params

d9ac834

flake8: dont shadow input

0c94dc2

i don't know what input is, but flake8-builtin says to not shadow it. okay.

override flake8-mypy default options

6ad523a

we provide missing types for py-libzfs, ucl, texttable, etc.

Release.file can be None

0a705c1

no need to initialize events' class vars with None

730dd57

BaseConfig: type our getters (and helpers)

e0b9b50

Jail/jid can be None (when the jail is not running)

e6ce2d7

Resource/config_type can be none

515419c

when the config doesn't exist

Resource/config_file can be None sometimes

4e8d75c

only use numeric values as sort_index

9226c11

Resource._dataset is not optional

537f9eb

typing for cli/set.py

82636bf

flake8: missing whitespace around -

08e8a93

remove various optional attributes and fix user defined defaults

abec92d

gronke force-pushed the flake8/more-tests branch from c795c8e to abec92d Compare September 19, 2017 19:45

gronke approved these changes Sep 19, 2017

View reviewed changes

igalic commented Sep 19, 2017

View reviewed changes

gronke merged commit 6e84749 into master Sep 19, 2017

gronke deleted the flake8/more-tests branch September 19, 2017 20:39

flake8: check for mutable function parameters #107

flake8: check for mutable function parameters #107

Uh oh!

Conversation

igalic commented Sep 17, 2017

Uh oh!

igalic commented Sep 17, 2017

Uh oh!

igalic Sep 19, 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!

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!

gronke Sep 19, 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!

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!

igalic Sep 19, 2017 •

edited

Loading

gronke Sep 19, 2017 •

edited

Loading