After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777216 - PEP 8 fix : __init__.py
PEP 8 fix : __init__.py
Status: RESOLVED FIXED
Product: gnome-music
Classification: Applications
Component: general
3.23.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-music-maint
gnome-music-maint
Depends on:
Blocks:
 
 
Reported: 2017-01-13 13:56 UTC by Abhinav Singh
Modified: 2017-01-16 09:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
PEP fixes (2.75 KB, patch)
2017-01-13 13:59 UTC, Abhinav Singh
none Details | Review
PEP 8 fixes and added docstrings (3.17 KB, patch)
2017-01-13 20:23 UTC, Abhinav Singh
committed Details | Review

Description Abhinav Singh 2017-01-13 13:56:43 UTC
Currently __init__.py does not follow all PEP guidelines.
Comment 1 Abhinav Singh 2017-01-13 13:59:40 UTC
Created attachment 343432 [details] [review]
PEP fixes
Comment 2 Abhinav Singh 2017-01-13 14:15:20 UTC
I believe singleton pattern is implemented with the TrackerWrapper.

The current usage is for example in window.py :>  
tracker = TrackerWrapper().tracker

Should I try to get rid of the constructor to make it look like this?
tracker = TrackerWrapper.tracker

It might require a meta class implementation.
Comment 3 Abhinav Singh 2017-01-13 14:25:00 UTC
Another idea would be to make it a module variable, like it was done with Grilo.
In the end of the grilo file, 
grilo = Grilo()

And then all other classes simply do
from gnomemusic.grilo, import grilo

So this will become like

from gnomemusic, import tracker
Comment 4 Marinus Schraal 2017-01-13 16:15:34 UTC
Review of attachment 343432 [details] [review]:

Some minor issues noted, but looking good.

::: gnomemusic/__init__.py
@@ +42,3 @@
+    Decorate functions with this to show their runtime info.
+    Use with the debug flag.
+    """

sphinx style, see other recent docstrings

@@ +56,3 @@
         if 'rateLimitedFunction' not in name:
             logger.debug("%s%s(%s)[%s:%s]",
+                         '|' * tabbing, name, params, filename, lineno)

As much as possible on one line, then break on the comma to the next.

@@ +66,2 @@
             elapsed_time = ', took %02f' % elapsed
         if elapsed_time or retval is not None:

multi statement if's in () with the operator on a new line up front

eg.
if (cond1
      and cond2):

@@ +68,3 @@
             if 'rateLimitedFunction' not in name:
+                logger.debug("%s  returned %s%s",
+                             '|' * tabbing, repr(retval), elapsed_time)

indentation

@@ +88,3 @@
             return repr(self)
+
+    __instance = None

_instance is fine
Comment 5 Marinus Schraal 2017-01-13 16:27:25 UTC
Regarding comments 2 & 3, it can stay as it is for now.
Comment 6 Abhinav Singh 2017-01-13 20:23:45 UTC
Created attachment 343449 [details] [review]
PEP 8 fixes and added docstrings
Comment 7 Marinus Schraal 2017-01-16 09:59:28 UTC
Made some minor enhancements and rewrote the commit message a bit.

Thanks for the patch.