Created attachment 343434 [details] [review]
PEP fixes

Review of attachment 343434 [details] [review]:

Just some minor notes.

::: gnomemusic/application.py
@@ +45,1 @@
 logger = logging.getLogger(__name__)

unused

@@ +57,2 @@
         GLib.set_prgname('gnome-music')
         self.settings = Gio.Settings.new('org.gnome.Music')

self._settings possible?

@@ +73,2 @@
+    @log
+    def do_startup(self):

Why reorder these? do_startup, quit & do_activate

@@ +77,3 @@
+        self._build_app_menu()
+
+    def do_activate(self):

@log

@@ +80,3 @@
+        if not self._window:
+            self._window = Window(self)
+            self.service = MediaPlayer2Service(self)

self._service, but since it isn't used it isn't needed really.

@@ +106,2 @@
     @log
+    def _new_playlist(self, action, param):

if it doesn't do anything, it can go. But make it a separate patch.

Created attachment 343483 [details] [review]
Cleanup - application.py

Followed PEP guidelines, and minor refactor of application.py


If I remove new_playlist action in a new patch, should I make it a patch of a patch?

Review of attachment 343483 [details] [review]:

almost done

::: gnomemusic/application.py
@@ +90,3 @@
     @log
+    def _about(self, action, param):
+        @log

I think inner function logging is a bit overkill.

@@ -116,3 @@
         if not self._window:
             self._window = Window(self)
-            self.service = MediaPlayer2Service(self)

I actually meant that it doesn't need to be assigned, it does need to be initialized. Actually I'm not 100% sure how the GC handles such a case, should check.

regarding comment #3 : I think just 2 patches stacking, remove the unused function first and then have the cleanup patch.

Created attachment 343550 [details] [review]
Removed unused functions.

Created attachment 343551 [details] [review]
Code clean up

Review of attachment 343550 [details] [review]:

lgtm

Review of attachment 343551 [details] [review]:

lgtm

Thanks for the patches. Committed.