[PATCH] Use abstract base classes for mixins.

David Shea dshea at redhat.com
Wed Dec 4 21:07:11 UTC 2013


This is only a beginning, but hopefully enough to get the idea across.
The purpose of this is twofold: For mixins/interfaces/whatever that do
not make sense unless inherited by another class, don't allow the class
to be instantiated. For mixins/interfaces/whatever that depend on
properties that are implemented in the inheriting class or its other
subclasses, make the relationship explicit through the use of abstract
properties.

SourceSwitchHandler is a good example, and the catalyst for this.
SourceSwitchHandler manipulates self.data, which is provided in the
subclass by way of EditTUISpoke -> UIObject. This kind of a
relationship is flagged by pylint since .data is not provided by
SourceSwitchHandler or any of its subclasses. b5df3a90 attempted to
remedy this by storing a copy of data in SourceSwitchHandler, but that
commit is incorrect: self.data in SourceSwitchHandler will resolve to
UIObject.data when called from a class that inherits from both, and
UIObject.data is a read-only property. This commit instead makes
SourceSwitchHandler an abstract base class. It implements a data
property of its own but requires that the property be overridden by
another class. This way no class can inherit from SourceSwitchHandler
without providing a data property.
---
 po/POTFILES.in                      |   1 +
 pyanaconda/ui/gui/spokes/storage.py |  36 +------
 pyanaconda/ui/interfaces.py         | 184 ++++++++++++++++++++++++++++++++++++
 pyanaconda/ui/tui/spokes/source.py  |  95 ++-----------------
 4 files changed, 196 insertions(+), 120 deletions(-)
 create mode 100644 pyanaconda/ui/interfaces.py

diff --git a/po/POTFILES.in b/po/POTFILES.in
index af263f9..c1d3a7e 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -28,6 +28,7 @@ pyanaconda/packaging/yumpayload.py
 
 # Interfaces
 pyanaconda/ui/common.py
+pyanaconda/ui/interfaces.py
 pyanaconda/ui/__init__.py
 
 # Common stuff
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py
index 4192a1a..ce80c72 100644
--- a/pyanaconda/ui/gui/spokes/storage.py
+++ b/pyanaconda/ui/gui/spokes/storage.py
@@ -50,6 +50,7 @@ from pyanaconda.ui.gui.spokes.lib.detailederror import DetailedErrorDialog
 from pyanaconda.ui.gui.spokes.lib.resize import ResizeDialog
 from pyanaconda.ui.gui.categories.system import SystemCategory
 from pyanaconda.ui.gui.utils import enlightbox, escape_markup
+from pyanaconda.ui.interfaces import StorageChecker
 
 from pyanaconda.kickstart import doKickstartStorage, getAvailableDiskSpace
 from blivet import empty_device
@@ -306,33 +307,6 @@ class InstallOptions3Dialog(InstallOptions1Dialog):
     def continue_response(self):
         return self.RESPONSE_CONTINUE_NONE
 
-class StorageChecker(object):
-    errors = []
-    warnings = []
-    _mainSpokeClass = "StorageSpoke"
-
-    def __init__(self):
-        # This is provided by the StorageSpoke class, which is a subclass of
-        # this one.  Backwards, I know.
-        self.storage  = None
-
-    def run(self):
-        threadMgr.add(AnacondaThread(name=constants.THREAD_CHECK_STORAGE,
-                                     target=self.checkStorage))
-
-    def checkStorage(self):
-        threadMgr.wait(constants.THREAD_EXECUTE_STORAGE)
-
-        hubQ.send_not_ready(self._mainSpokeClass)
-        hubQ.send_message(self._mainSpokeClass, _("Checking storage configuration..."))
-        (StorageChecker.errors,
-         StorageChecker.warnings) = self.storage.sanityCheck()
-        hubQ.send_ready(self._mainSpokeClass, True)
-        for e in StorageChecker.errors:
-            log.error(e)
-        for w in StorageChecker.warnings:
-            log.warn(w)
-
 class StorageSpoke(NormalSpoke, StorageChecker):
     builderObjects = ["storageWindow", "addSpecializedImage"]
     mainWidgetName = "storageWindow"
@@ -431,7 +405,7 @@ class StorageSpoke(NormalSpoke, StorageChecker):
             doKickstartStorage(self.storage, self.data, self.instclass)
         except (StorageError, KickstartValueError) as e:
             log.error("storage configuration failed: %s", e)
-            StorageChecker.errors = str(e).split("\n")
+            self.errors = str(e).split("\n")
             hubQ.send_message(self.__class__.__name__, _("Failed to save storage configuration..."))
             self.data.bootloader.bootDrive = ""
             self.data.ignoredisk.drives = []
@@ -443,14 +417,14 @@ class StorageSpoke(NormalSpoke, StorageChecker):
             self._applyDiskSelection(self.selected_disks)
         except BootLoaderError as e:
             log.error("BootLoader setup failed: %s", e)
-            StorageChecker.errors = str(e).split("\n")
+            self.errors = str(e).split("\n")
             hubQ.send_message(self.__class__.__name__, _("Failed to save storage configuration..."))
             self.data.bootloader.bootDrive = ""
         else:
             if self.autopart:
                 # this was already run as part of doAutoPartition. dumb.
-                StorageChecker.errors = []
-                StorageChecker.warnings = []
+                self.errors = []
+                self.warnings = []
                 self.run()
         finally:
             self._ready = True
diff --git a/pyanaconda/ui/interfaces.py b/pyanaconda/ui/interfaces.py
new file mode 100644
index 0000000..6b46d03
--- /dev/null
+++ b/pyanaconda/ui/interfaces.py
@@ -0,0 +1,184 @@
+# Abstract interfaces for UI classes
+#
+# Copyright (C) 2013  Red Hat, Inc.
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions of
+# the GNU General Public License v.2, or (at your option) any later version.
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY expressed or implied, including the implied warranties of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
+# Public License for more details.  You should have received a copy of the
+# GNU General Public License along with this program; if not, write to the
+# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.  Any Red Hat trademarks that are incorporated in the
+# source code or documentation are not subject to the GNU General Public
+# License and may only be used or replicated with the express permission of
+# Red Hat, Inc.
+#
+# Red Hat Author(s): David Shea <dshea at redhat.com
+#
+
+# This file contains abstract base classes that provide specific functionality
+# that can be added to another class. The idea is sort-of modelled after Java's
+# interfaces. An abstract base class cannot be instatiated, and it provides a
+# contract for classes that inherit from it: any method or property marked as
+# abstract in the base class must be overriden in the inheriting class. This
+# allows for cleaner implementation of certain types of mixin-classes: a class
+# that adds functionality to another class can explicitly require that methods
+# or properties be provided by the inheriting class or another superclass of
+# the inheriting class.
+#
+# In general, classes that inherit from abstract base classes should place the
+# abstract base class at the end of the inheritance list. This way any abstract
+# methods or properties in the abc will be overridden by the base classes
+# that are first in the inheritance list. For example, an abstract base class
+# may add a method that reads from spoke.data:
+#
+#    class Mixin(object):
+#        __metaclass__ = ABCMeta
+#
+#        @abstractproperty
+#        def data(self): pass
+#
+#        def isHD(self):
+#            return self.data.method == "harddrive"
+#
+# The Mixin class will add the method isHD to any class that inherits from it,
+# and classes that inherit from Mixin must provide a data property.
+#
+#    class MixedObject(UIObject, Mixin):
+#        ....
+#
+# The method resolution order of MixedObject resolves UIObject.data before
+# Mixin.data, so UIObject.data satisfies the requirment that Mixin.data be
+# overriden.
+
+from abc import ABCMeta, abstractproperty
+
+from pyanaconda import constants
+from pyanaconda.threads import threadMgr, AnacondaThread
+from pyanaconda.ui.communication import hubQ
+from pyanaconda.i18n import _
+
+import logging
+import copy
+
+class StorageChecker(object):
+    __metaclass__ = ABCMeta
+
+    log = logging.getLogger("anaconda")
+
+    def __init__(self):
+        self.errors = []
+        self.warnings = []
+
+    @abstractproperty
+    def storage(self): pass
+
+    def run(self):
+        threadMgr.add(AnacondaThread(name=constants.THREAD_CHECK_STORAGE,
+                                     target=self.checkStorage))
+
+    def checkStorage(self):
+        threadMgr.wait(constants.THREAD_EXECUTE_STORAGE)
+
+        hubQ.send_not_read(self.__class__.__name__)
+        hubQ.send_message(self.__class__.__name__, _("Check storage configuration..."))
+        (self.errors,
+         self.warnings) = self.storage.sanityCheck()
+        for e in self.errors:
+            self.log.error(e)
+        for w in self.warnings:
+            self.log.warn(w)
+
+class SourceSwitchHandler(object):
+    """ A class that can be used as a mixin handling
+    installation source switching.
+    It will correctly switch to the new method
+    and cleanup any previous method set.
+    """
+
+    __metaclass__ = ABCMeta
+
+    @abstractproperty
+    def data(self): pass
+
+    @abstractproperty
+    def storage(self): pass
+
+    def __init__(self):
+        self._device = None
+        self._current_iso_path = None
+
+    def _clean_hdd_iso(self):
+        """ Clean HDD ISO usage
+        This means unmounting the partition and unprotecting it,
+        so it can be used for the installation.
+        """
+        if self.data.method.method == "harddrive" and self.data.method.partition:
+            part = self.data.method.partition
+            dev = self.storage.devicetree.getDeviceByName(part)
+            if dev:
+                dev.protected = False
+            self.storage.config.protectedDevSpecs.remove(part)
+
+    def set_source_hdd_iso(self, device, iso_path):
+        """ Switch to the HDD ISO install source
+        :param partition: name of the partition hosting the ISO
+        :type partition: string
+        :param iso_path: full path to the source ISO file
+        :type iso_path: string
+        """
+        partition = device.name
+        # the GUI source spoke also does the copy
+        old_source = copy.copy(self.data.method)
+
+        # if a different partition was used previously, unprotect it
+        if old_source.method == "harddrive" and old_source.partition != partition:
+            self._clean_hdd_iso()
+
+        # protect current device
+        if device:
+            device.protected = True
+            self.storage.config.protectedDevSpecs.append(device.name)
+
+        self.data.method.method = "harddrive"
+        self.data.method.partition = partition
+        # the / gets stripped off by payload.ISOImage
+        self.data.method.dir = "/" + iso_path
+
+        # as we already made the device protected when
+        # switching to it, we don't need to protect it here
+
+    def set_source_url(self, url=None):
+        """ Switch to install source specified by URL """
+        # clean any old HDD ISO sources
+        self._clean_hdd_iso()
+
+        self.data.method.method = "url"
+        if url is not None:
+            self.data.method.url = url
+
+    def set_source_nfs(self, opts=None):
+        """ Switch to NFS install source """
+        # clean any old HDD ISO sources
+        self._clean_hdd_iso()
+
+        self.data.method.method = "nfs"
+        if opts is not None:
+            self.data.method.opts = opts
+
+    def set_source_cdrom(self):
+        """ Switch to cdrom install source """
+        # clean any old HDD ISO sources
+        self._clean_hdd_iso()
+
+        self.data.method.method = "cdrom"
+
+    def set_source_closest_mirror(self):
+        """ Switch to the closest mirror install source """
+        # clean any old HDD ISO sources
+        self._clean_hdd_iso()
+
+        self.data.method.method = None
diff --git a/pyanaconda/ui/tui/spokes/source.py b/pyanaconda/ui/tui/spokes/source.py
index 977e7df..a3397fd 100644
--- a/pyanaconda/ui/tui/spokes/source.py
+++ b/pyanaconda/ui/tui/spokes/source.py
@@ -33,12 +33,13 @@ from pyanaconda.constants import THREAD_PAYLOAD_MD, THREAD_STORAGE, THREAD_STORA
 from pyanaconda.constants import THREAD_CHECK_SOFTWARE, ISO_DIR, DRACUT_ISODIR
 from pyanaconda.constants_text import INPUT_PROCESSED
 
+from pyanaconda.ui.interfaces import SourceSwitchHandler
+
 from blivet.util import get_mount_paths
 
 import re
 import os
 import fnmatch
-import copy
 
 import logging
 LOG = logging.getLogger("anaconda")
@@ -46,90 +47,6 @@ LOG = logging.getLogger("anaconda")
 
 __all__ = ["SourceSpoke"]
 
-class SourceSwitchHandler(object):
-    """ A class that can be used as a mixin handling
-    installation source switching.
-    It will correctly switch to the new method
-    and cleanup any previous method set.
-    """
-    def __init__(self, data, storage):
-        self._device = None
-        self._current_iso_path = None
-        self.data = data
-        self.storage = storage
-
-    def _clean_hdd_iso(self):
-        """ Clean HDD ISO usage
-        This means unmounting the partition and unprotecting it,
-        so it can be used for the installation.
-        """
-        if self.data.method.method == "harddrive" and self.data.method.partition:
-            part = self.data.method.partition
-            dev = self.storage.devicetree.getDeviceByName(part)
-            if dev:
-                dev.protected = False
-            self.storage.config.protectedDevSpecs.remove(part)
-
-    def set_source_hdd_iso(self, device, iso_path):
-        """ Switch to the HDD ISO install source
-        :param partition: name of the partition hosting the ISO
-        :type partition: string
-        :param iso_path: full path to the source ISO file
-        :type iso_path: string
-        """
-        partition = device.name
-        # the GUI source spoke also does the copy
-        old_source = copy.copy(self.data.method)
-
-        # if a different partition was used previously, unprotect it
-        if old_source.method == "harddrive" and old_source.partition != partition:
-            self._clean_hdd_iso()
-
-        # protect current device
-        if device:
-            device.protected = True
-            self.storage.config.protectedDevSpecs.append(device.name)
-
-        self.data.method.method = "harddrive"
-        self.data.method.partition = partition
-        # the / gets stripped off by payload.ISOImage
-        self.data.method.dir = "/" + iso_path
-
-        # as we already made the device protected when
-        # switching to it, we don't need to protect it here
-
-    def set_source_url(self, url=None):
-        """ Switch to install source specified by URL """
-        # clean any old HDD ISO sources
-        self._clean_hdd_iso()
-
-        self.data.method.method = "url"
-        if url is not None:
-            self.data.method.url = url
-
-    def set_source_nfs(self, opts=None):
-        """ Switch to NFS install source """
-        # clean any old HDD ISO sources
-        self._clean_hdd_iso()
-
-        self.data.method.method = "nfs"
-        if opts is not None:
-            self.data.method.opts = opts
-
-    def set_source_cdrom(self):
-        """ Switch to cdrom install source """
-        # clean any old HDD ISO sources
-        self._clean_hdd_iso()
-
-        self.data.method.method = "cdrom"
-
-    def set_source_closest_mirror(self):
-        """ Switch to the closest mirror install source """
-        # clean any old HDD ISO sources
-        self._clean_hdd_iso()
-
-        self.data.method.method = None
-
 class SourceSpoke(EditTUISpoke, SourceSwitchHandler):
     """ Spoke used to customize the install source repo. """
     title = N_("Installation source")
@@ -142,7 +59,7 @@ class SourceSpoke(EditTUISpoke, SourceSwitchHandler):
 
     def __init__(self, app, data, storage, payload, instclass):
         EditTUISpoke.__init__(self, app, data, storage, payload, instclass)
-        SourceSwitchHandler.__init__(self, data, storage)
+        SourceSwitchHandler.__init__(self)
         self._ready = False
         self.errors = []
         self._cdrom = None
@@ -344,7 +261,7 @@ class SpecifyRepoSpoke(EditTUISpoke, SourceSwitchHandler):
 
     def __init__(self, app, data, storage, payload, instclass, selection):
         EditTUISpoke.__init__(self, app, data, storage, payload, instclass)
-        SourceSwitchHandler.__init__(self, data, storage)
+        SourceSwitchHandler.__init__(self)
         self.selection = selection
         self.args = self.data.method
 
@@ -383,7 +300,7 @@ class SpecifyNFSRepoSpoke(EditTUISpoke, SourceSwitchHandler):
 
     def __init__(self, app, data, storage, payload, instclass, selection, errors):
         EditTUISpoke.__init__(self, app, data, storage, payload, instclass)
-        SourceSwitchHandler.__init__(self, data, storage)
+        SourceSwitchHandler.__init__(self)
         self.selection = selection
         self.args = self.data.method
         self.errors = errors
@@ -504,7 +421,7 @@ class SelectISOSpoke(NormalTUISpoke, SourceSwitchHandler):
 
     def __init__(self, app, data, storage, payload, instclass, device):
         NormalTUISpoke.__init__(self, app, data, storage, payload, instclass)
-        SourceSwitchHandler.__init__(self, data, storage)
+        SourceSwitchHandler.__init__(self)
         self.selection = None
         self.args = self.data.method
         self._device = device
-- 
1.8.4.2



More information about the anaconda-patches mailing list