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.

_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

Reply via email to