GNOME Bugzilla – Bug 774500
Refactor gnome-music.in to be more readable
Last modified: 2017-04-14 13:00:14 UTC
gnome-music.in is the main entry point for gnome-music. It looks like it has grew up organically and it is somekind hard to understand now. Observing the commit history for this script, it is possible to see that it started with a marely trigger for gnome-music Gtk.Application and it has been inflated with many configurations (such as resources, libgd, i18n, etc...). Following the wave of cleaning modules according to PEP-8, I think gnome-music could also be organised to group this configurations within functions, such as pitivi.in (https://github.com/pitivi/pitivi/blob/master/bin/pitivi.in), so it could be more easy to read, maintain and understand what is happening.
Created attachment 339986 [details] [review] gnome-music: groups configurations within functions gnome-music.in was refactored to be more readable. All pre-configurations that are need to be made before starting gnome-music were grouped in proper functions so it is now easies to read, understand and maintain. It also follow PEP-8 where it is possible.
Created attachment 340524 [details] [review] gnome-music: groups configurations within functions gnome-music.in was refactored to be more readable. All pre-configurations that are need to be made before starting gnome-music were grouped in proper functions so it is now easies to read, understand and maintain. It also follow PEP-8 where it is possible.
Review of attachment 340524 [details] [review]: I'm pretty sure this is alright, but I haven't had the time to check it all. Nor am I sure what every element is supposed to do and how it might interact. So I think we're just gonna put in for the next dev cycle and see if anything comes up. Some minor indentation fixes and this should be good to go. ::: gnome-music.in @@ +100,3 @@ parser = argparse.ArgumentParser() + parser.add_argument( + '-d', '--debug', action='store_true', default=False, dest='debug') for indentation here I prefer indenting on the opening parenthesis if possible with the length of the arguments (the review tool isn't good with indentation examples like this :( ) : function(arg1, arg2, arg3, arg4) @@ +104,3 @@ if args.debug: + logging.basicConfig( + level=logging.DEBUG, format=LOG_FORMAT, datefmt=LOG_DATE_FORMAT) same @@ +112,3 @@ else: + logging.basicConfig( + level=logging.WARN, format=LOG_FORMAT, datefmt=LOG_DATE_FORMAT) same
Created attachment 346939 [details] [review] gnome-music.in: groups configurations within functions All pre-configurations that must be set before starting gnome-music were grouped in proper functions so it is now easier to read, understand and maintain. It also follows PEP-8 when possible.
Review of attachment 346939 [details] [review]: lgtm ::: gnome-music.in @@ +60,3 @@ + + +# Log settigns typo & keep consistent: some comments do a white line after the comment, others don't. I'm in favor of no whitespace in this case.
Created attachment 349536 [details] [review] gnome-music.in: groups configurations within functions All pre-configurations that must be set before starting gnome-music were grouped in proper functions so it is now easier to read, understand and maintain. It also follows PEP-8 when possible.
Review of attachment 349536 [details] [review]: lgtm
Fixed a mistake with the pkgdatadir being set from the localedir. Thanks for the patch!