openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

Sorry, a few nit picks...

Diff comments:

> === modified file 'openlp/core/app.py'
> --- openlp/core/app.py	2019-03-28 21:03:32 +0000
> +++ openlp/core/app.py	2019-04-30 19:47:11 +0000
> @@ -301,6 +302,8 @@
>                          help='Set logging to LEVEL level. Valid values are "debug", "info", "warning".')
>      parser.add_argument('-p', '--portable', dest='portable', action='store_true',
>                          help='Specify if this should be run as a portable app, ')
> +    parser.add_argument('-pp', '--portable-path', dest='portablepath', default=None,
> +                        help='Specify the path of the portable data, defaults to "<AppDir>/../../".')

we have *some* window users can you make this:

'Specify the path of the portable data, defaults to "{dir_name}".'.format(dir_name=os.path.join('<AppDir>', '..', '..'))

>      parser.add_argument('-w', '--no-web-server', dest='no_web_server', action='store_true',
>                          help='Turn off the Web and Socket Server ')
>      parser.add_argument('rargs', nargs='?', default=[])
> @@ -356,7 +359,14 @@
>          application.setApplicationName('OpenLPPortable')
>          Settings.setDefaultFormat(Settings.IniFormat)
>          # Get location OpenLPPortable.ini
> -        portable_path = (AppLocation.get_directory(AppLocation.AppDir) / '..' / '..').resolve()
> +        if args.portablepath:
> +            if os.path.isabs(args.portablepath):
> +                portable_path = str_to_path(args.portablepath).resolve()

str_to_path isn't required here. str_to_path('') == None, Path('') == working directory.
The line above (if args.portablepath:) already ensures that you're not passing an empty str.

Make it:
+                portable_path = Path(args.portablepath)

> +            else:
> +                portable_path = (AppLocation.get_directory(AppLocation.AppDir) / '..' /
> +                                 str_to_path(args.portablepath)).resolve()

No need for str_to_path here either, args.portablepath is a string, using the '/' operator way of joining paths can take os.pathlike or string objects.

Make it:
+                                 AppLocation.get_directory(AppLocation.AppDir) / '..' / args.portablepath

> +        else:
> +            portable_path = (AppLocation.get_directory(AppLocation.AppDir) / '..' / '..').resolve()

As with the above two comments move resolve() down.

Make it:
+            portable_path = AppLocation.get_directory(AppLocation.AppDir) / '..' / '..'

Finally, just the tidy the above up move each ".resolve()" down here:
+         portable_path = portable_path.resolve()

>          data_path = portable_path / 'Data'
>          set_up_logging(portable_path / 'Other')
>          log.info('Running portable')


-- 
https://code.launchpad.net/~tomasgroth/openlp/portable-path/+merge/366734
Your team OpenLP Core is subscribed to branch lp:openlp.