Merge lp:~phill-ridout/openlp/import-depreciations into lp:openlp

[Phill]

Fix the depreciated code, and refactor it.

Add this to your merge proposal:
--------------------------------
lp:~phill-ridout/openlp/import-depreciations (revision 2736)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/2007/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1917/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1853/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Code_Analysis/1233/
[FAILURE] https://ci.openlp.io/job/Branch-04b-Test_Coverage/1092/
Stopping after failure

[Tim Bentley (trb143)]

Looks good but has issues with trunk!

review: Needs Fixing

[lp:~phill-ridout/openlp/import-depreciations]

2733. By Phill on 2017-05-14

Update and refactor dynamic import code

2734. By Phill on 2017-05-15

Reworked the extension_loader function

2735. By Phill on 2017-05-15

head

2736. By Phill on 2017-05-15

pep

Unmerged revisions

=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2016-12-31 11:01:36 +0000
+++ openlp/core/common/__init__.py	2017-05-14 07:11:54 +0000
@@ -23,8 +23,9 @@
 The :mod:`common` module contains most of the components and libraries that make
 OpenLP work.
 """
+import glob
 import hashlib
-
+import importlib
 import logging
 import os
 import re
@@ -79,6 +80,38 @@
             log.exception('failed to check if directory exists or create directory')
 
 
+def extension_loader(glob_pattern, excluded_files=[]):
+    """
+    A utility function to find and load OpenLP extensions, such as plugins, presentation and media controllers and
+    importers.
+
+    :param glob_pattern: A glob pattern used to find the extension(s) to be imported.
+        i.e. openlp_app_dir/plugins/*/*plugin.py
+    :type glob_pattern: str
+    :param excluded_files: A list of file names to exclude that the glob pattern may find.
+    :type excluded_files: list of strings
+
+    :return: None
+    :rtype: None
+    """
+    for extension_path in glob.iglob(glob_pattern):
+        filename = os.path.split(extension_path)[1]
+        if filename in excluded_files:
+            continue
+        module_name = os.path.splitext(filename)[0]
+        try:
+            loader = importlib.machinery.SourceFileLoader(module_name, extension_path)
+            loader.load_module()
+            # TODO: A better way to do this (once we drop python 3.4 support)
+            # spec = importlib.util.spec_from_file_location('what.ever', 'foo.py')
+            # module = importlib.util.module_from_spec(spec)
+            # spec.loader.exec_module(module)
+        except (ImportError, OSError):
+            # On some platforms importing vlc.py might cause OSError exceptions. (e.g. Mac OS X)
+            log.warning('Failed to import {module_name} on path {extension_path}'
+                        .format(module_name=module_name, extension_path=extension_path))
+
+
 def get_frozen_path(frozen_option, non_frozen_option):
     """
     Return a path based on the system status.

=== modified file 'openlp/core/lib/pluginmanager.py'
--- openlp/core/lib/pluginmanager.py	2016-12-31 11:01:36 +0000
+++ openlp/core/lib/pluginmanager.py	2017-05-14 07:11:54 +0000
@@ -23,10 +23,9 @@
 Provide plugin management
 """
 import os
-import imp
 
 from openlp.core.lib import Plugin, PluginStatus
-from openlp.core.common import AppLocation, RegistryProperties, OpenLPMixin, RegistryMixin
+from openlp.core.common import AppLocation, RegistryProperties, OpenLPMixin, RegistryMixin, extension_loader
 
 
 class PluginManager(RegistryMixin, OpenLPMixin, RegistryProperties):
@@ -70,32 +69,8 @@
         """
         Scan a directory for objects inheriting from the ``Plugin`` class.
         """
-        start_depth = len(os.path.abspath(self.base_path).split(os.sep))
-        present_plugin_dir = os.path.join(self.base_path, 'presentations')
-        self.log_debug('finding plugins in {path} at depth {depth:d}'.format(path=self.base_path, depth=start_depth))
-        for root, dirs, files in os.walk(self.base_path):
-            for name in files:
-                if name.endswith('.py') and not name.startswith('__'):
-                    path = os.path.abspath(os.path.join(root, name))
-                    this_depth = len(path.split(os.sep))
-                    if this_depth - start_depth > 2:
-                        # skip anything lower down
-                        break
-                    module_name = name[:-3]
-                    # import the modules
-                    self.log_debug('Importing {name} from {root}. Depth {depth:d}'.format(name=module_name,
-                                                                                          root=root,
-                                                                                          depth=this_depth))
-                    try:
-                        # Use the "imp" library to try to get around a problem with the PyUNO library which
-                        # monkey-patches the __import__ function to do some magic. This causes issues with our tests.
-                        # First, try to find the module we want to import, searching the directory in root
-                        fp, path_name, description = imp.find_module(module_name, [root])
-                        # Then load the module (do the actual import) using the details from find_module()
-                        imp.load_module(module_name, fp, path_name, description)
-                    except ImportError as e:
-                        self.log_exception('Failed to import module {name} on path {path}: '
-                                           '{args}'.format(name=module_name, path=path, args=e.args[0]))
+        glob_pattern = os.path.join(self.base_path, '*', '*plugin.py')
+        extension_loader(glob_pattern)
         plugin_classes = Plugin.__subclasses__()
         plugin_objects = []
         for p in plugin_classes:

=== modified file 'openlp/core/ui/media/mediacontroller.py'
--- openlp/core/ui/media/mediacontroller.py	2016-12-31 11:01:36 +0000
+++ openlp/core/ui/media/mediacontroller.py	2017-05-14 07:11:54 +0000
@@ -28,7 +28,8 @@
 import datetime
 from PyQt5 import QtCore, QtWidgets
 
-from openlp.core.common import OpenLPMixin, Registry, RegistryMixin, RegistryProperties, Settings, UiStrings, translate
+from openlp.core.common import OpenLPMixin, Registry, RegistryMixin, RegistryProperties, Settings, UiStrings, \
+    extension_loader, translate
 from openlp.core.lib import ItemCapabilities
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.core.common import AppLocation
@@ -39,6 +40,7 @@
     parse_optical_path
 from openlp.core.ui.lib.toolbar import OpenLPToolbar
 
+
 log = logging.getLogger(__name__)
 
 TICK_TIME = 200
@@ -173,18 +175,8 @@
         """
         log.debug('_check_available_media_players')
         controller_dir = os.path.join(AppLocation.get_directory(AppLocation.AppDir), 'core', 'ui', 'media')
-        for filename in os.listdir(controller_dir):
-            if filename.endswith('player.py') and filename != 'mediaplayer.py':
-                path = os.path.join(controller_dir, filename)
-                if os.path.isfile(path):
-                    module_name = 'openlp.core.ui.media.' + os.path.splitext(filename)[0]
-                    log.debug('Importing controller %s', module_name)
-                    try:
-                        __import__(module_name, globals(), locals(), [])
-                    # On some platforms importing vlc.py might cause
-                    # also OSError exceptions. (e.g. Mac OS X)
-                    except (ImportError, OSError):
-                        log.warning('Failed to import %s on path %s', module_name, path)
+        glob_pattern = os.path.join(controller_dir, '*player.py')
+        extension_loader(glob_pattern, ['mediaplayer.py'])
         player_classes = MediaPlayer.__subclasses__()
         for player_class in player_classes:
             self.register_players(player_class(self))

=== modified file 'openlp/plugins/presentations/lib/impresscontroller.py'
--- openlp/plugins/presentations/lib/impresscontroller.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/lib/impresscontroller.py	2017-05-14 07:11:54 +0000
@@ -58,7 +58,8 @@
 
 from openlp.core.lib import ScreenList
 from openlp.core.common import get_uno_command, get_uno_instance
-from .presentationcontroller import PresentationController, PresentationDocument, TextType
+from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument, \
+    TextType
 
 
 log = logging.getLogger(__name__)

=== modified file 'openlp/plugins/presentations/lib/pdfcontroller.py'
--- openlp/plugins/presentations/lib/pdfcontroller.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/lib/pdfcontroller.py	2017-05-14 07:11:54 +0000
@@ -29,7 +29,7 @@
 from openlp.core.common import AppLocation, check_binary_exists
 from openlp.core.common import Settings, is_win
 from openlp.core.lib import ScreenList
-from .presentationcontroller import PresentationController, PresentationDocument
+from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument
 
 if is_win():
     from subprocess import STARTUPINFO, STARTF_USESHOWWINDOW

=== modified file 'openlp/plugins/presentations/lib/powerpointcontroller.py'
--- openlp/plugins/presentations/lib/powerpointcontroller.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/lib/powerpointcontroller.py	2017-05-14 07:11:54 +0000
@@ -43,7 +43,7 @@
 from openlp.core.lib import ScreenList
 from openlp.core.lib.ui import UiStrings, critical_error_message_box, translate
 from openlp.core.common import trace_error_handler, Registry
-from .presentationcontroller import PresentationController, PresentationDocument
+from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument
 
 log = logging.getLogger(__name__)
 

=== modified file 'openlp/plugins/presentations/lib/pptviewcontroller.py'
--- openlp/plugins/presentations/lib/pptviewcontroller.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/lib/pptviewcontroller.py	2017-05-14 07:11:54 +0000
@@ -35,7 +35,7 @@
 
 from openlp.core.common import AppLocation
 from openlp.core.lib import ScreenList
-from .presentationcontroller import PresentationController, PresentationDocument
+from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument
 
 
 log = logging.getLogger(__name__)

=== modified file 'openlp/plugins/presentations/lib/presentationtab.py'
--- openlp/plugins/presentations/lib/presentationtab.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/lib/presentationtab.py	2017-05-14 07:11:54 +0000
@@ -25,7 +25,7 @@
 from openlp.core.common import Settings, UiStrings, translate
 from openlp.core.lib import SettingsTab, build_icon
 from openlp.core.lib.ui import critical_error_message_box
-from .pdfcontroller import PdfController
+from openlp.plugins.presentations.lib.pdfcontroller import PdfController
 
 
 class PresentationTab(SettingsTab):

=== modified file 'openlp/plugins/presentations/presentationplugin.py'
--- openlp/plugins/presentations/presentationplugin.py	2016-12-31 11:01:36 +0000
+++ openlp/plugins/presentations/presentationplugin.py	2017-05-14 07:11:54 +0000
@@ -20,19 +20,18 @@
 # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
 ###############################################################################
 """
-The :mod:`presentationplugin` module provides the ability for OpenLP to display presentations from a variety of document
-formats.
+The :mod:`openlp.plugins.presentations.presentationplugin` module provides the ability for OpenLP to display
+presentations from a variety of document formats.
 """
 import os
 import logging
 
 from PyQt5 import QtCore
 
-from openlp.core.common import AppLocation, translate
+from openlp.core.common import AppLocation, extension_loader, translate
 from openlp.core.lib import Plugin, StringContent, build_icon
 from openlp.plugins.presentations.lib import PresentationController, PresentationMediaItem, PresentationTab
 
-
 log = logging.getLogger(__name__)
 
 
@@ -123,16 +122,8 @@
         """
         log.debug('check_pre_conditions')
         controller_dir = os.path.join(AppLocation.get_directory(AppLocation.PluginsDir), 'presentations', 'lib')
-        for filename in os.listdir(controller_dir):
-            if filename.endswith('controller.py') and filename != 'presentationcontroller.py':
-                path = os.path.join(controller_dir, filename)
-                if os.path.isfile(path):
-                    module_name = 'openlp.plugins.presentations.lib.' + os.path.splitext(filename)[0]
-                    log.debug('Importing controller {name}'.format(name=module_name))
-                    try:
-                        __import__(module_name, globals(), locals(), [])
-                    except ImportError:
-                        log.warning('Failed to import {name} on path {path}'.format(name=module_name, path=path))
+        glob_pattern = os.path.join(controller_dir, '*controller.py')
+        extension_loader(glob_pattern, ['presentationcontroller.py'])
         controller_classes = PresentationController.__subclasses__()
         for controller_class in controller_classes:
             controller = controller_class(self)

=== modified file 'tests/functional/openlp_core_common/test_common.py'
--- tests/functional/openlp_core_common/test_common.py	2017-04-24 05:17:55 +0000
+++ tests/functional/openlp_core_common/test_common.py	2017-05-14 07:11:54 +0000
@@ -23,10 +23,10 @@
 Functional tests to test the AppLocation class and related methods.
 """
 from unittest import TestCase
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import check_directory_exists, de_hump, trace_error_handler, translate, is_win, is_macosx, \
-    is_linux, clean_button_text
+from openlp.core.common import check_directory_exists, clean_button_text, de_hump, extension_loader, is_macosx, \
+    is_linux, is_win, trace_error_handler, translate
 
 
 class TestCommonFunctions(TestCase):
@@ -72,6 +72,68 @@
             mocked_exists.assert_called_with(directory_to_check)
             self.assertRaises(ValueError, check_directory_exists, directory_to_check)
 
+    def test_extension_loader_no_files_found(self):
+        """
+        Test the `extension_loader` function when no files are found
+        """
+        # GIVEN: A mocked `iglob` function which does not match any files
+        with patch('openlp.core.common.glob.iglob', return_value=[]), \
+                patch('openlp.core.common.importlib.machinery.SourceFileLoader') as mocked_source_file_loader:
+
+            # WHEN: Calling `extension_loader`
+            extension_loader('glob', ['file2.py', 'file3.py'])
+
+            # THEN: `extension_loader` should not try to import any files
+            self.assertFalse(mocked_source_file_loader.called)
+
+    def test_extension_loader_files_found(self):
+        """
+        Test the `extension_loader` function when it successfully finds and loads some files
+        """
+        # GIVEN: A mocked `iglob` function which returns a list of files
+        with patch('openlp.core.common.glob.iglob', return_value=['import_dir/file1.py', 'import_dir/file2.py',
+                                                                  'import_dir/file3.py', 'import_dir/file4.py']), \
+                patch('openlp.core.common.importlib.machinery.SourceFileLoader') as mocked_source_file_loader:
+
+            # WHEN: Calling `extension_loader` with a list of files to exclude
+            extension_loader('glob', ['file2.py', 'file3.py'])
+
+            # THEN: `extension_loader` should only try to import the files that are matched by the blob, excluding the
+            #       files listed in the `excluded_files` argument
+            mocked_source_file_loader.assert_has_calls([call('file1', 'import_dir/file1.py'), call().load_module(),
+                                                        call('file4', 'import_dir/file4.py'), call().load_module()])
+
+    def test_extension_loader_import_error(self):
+        """
+        Test the `extension_loader` function when `SourceFileLoader` raises a `ImportError`
+        """
+        # GIVEN: A mocked `SourceFileLoader` which raises an `ImportError`
+        with patch('openlp.core.common.glob.iglob', return_value=['import_dir/file1.py', 'import_dir/file2.py',
+                                                                  'import_dir/file3.py', 'import_dir/file4.py']), \
+                patch('openlp.core.common.importlib.machinery.SourceFileLoader', side_effect=ImportError()), \
+                patch('openlp.core.common.log') as mocked_logger:
+
+            # WHEN: Calling `extension_loader`
+            extension_loader('glob')
+
+            # THEN: The `ImportError` should be caught and logged
+            self.assertTrue(mocked_logger.warning.called)
+
+    def test_extension_loader_os_error(self):
+        """
+        Test the `extension_loader` function when `SourceFileLoader` raises a `ImportError`
+        """
+        # GIVEN: A mocked `SourceFileLoader` which raises an `OSError`
+        with patch('openlp.core.common.glob.iglob', return_value=['import_dir/file1.py']), \
+                patch('openlp.core.common.importlib.machinery.SourceFileLoader', side_effect=OSError()), \
+                patch('openlp.core.common.log') as mocked_logger:
+
+            # WHEN: Calling `extension_loader`
+            extension_loader('glob')
+
+            # THEN: The `OSError` should be caught and logged
+            self.assertTrue(mocked_logger.warning.called)
+
     def test_de_hump_conversion(self):
         """
         Test the de_hump function with a class name