openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

Looks good, just 2 things:

1. Merge from trunk, it'll fix the tests
2. I have some comments on your import statements in-line, please fix them up.

Diff comments:

> 

Don't import things you aren't going to use in this module. We've got some really terrible imports (that I've been sorting out in my refactors), and one way of avoiding that mess is via "direct" or "fully qualified" imports. So, instead of "from openlp.core.projectors import PJLink", use "from openlp.core.projectors.pjlink import PJLink".

> === renamed file 'openlp/core/lib/projector/constants.py' => 'openlp/core/projectors/constants.py'
> === renamed file 'openlp/core/lib/projector/db.py' => 'openlp/core/projectors/db.py'
> 
> === renamed file 'openlp/core/lib/projector/upgrade.py' => 'openlp/core/projectors/upgrade.py'
> === modified file 'openlp/core/ui/__init__.py'
> --- openlp/core/ui/__init__.py	2017-06-05 18:22:14 +0000
> +++ openlp/core/ui/__init__.py	2017-11-10 12:12:52 +0000
> @@ -115,9 +115,10 @@
>  from .shortcutlistform import ShortcutListForm
>  from .servicemanager import ServiceManager
>  from .thememanager import ThemeManager
> -from .projector.manager import ProjectorManager
> -from .projector.tab import ProjectorTab
> -from .projector.editform import ProjectorEditForm
> +
> +from openlp.core.projectors import ProjectorManager
> +from openlp.core.projectors import ProjectorTab
> +from openlp.core.projectors import ProjectorEditForm

Please don't do this. 1. You have 3 objects from the same module, which could be imported on the same line as "from a import b, c, d". 2. As mentioned above, please use fully qualified imports, "from openlp.core.projects.tab import ProjectorTab"

>  
>  __all__ = ['SplashScreen', 'AboutForm', 'SettingsForm', 'MainDisplay', 'SlideController', 'ServiceManager', 'ThemeForm',
>             'ThemeManager', 'ServiceItemEditForm', 'FirstTimeForm', 'FirstTimeLanguageForm', 'Display', 'AudioPlayer',


-- 
https://code.launchpad.net/~alisonken1/openlp/pjlink2-l/+merge/333537
Your team OpenLP Core is subscribed to branch lp:openlp.