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 773478 - Cleaning artistsview.py according to PEP-8 standards
Cleaning artistsview.py according to PEP-8 standards
Status: RESOLVED FIXED
Product: gnome-music
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-music-maint
gnome-music-maint
Depends on:
Blocks:
 
 
Reported: 2016-10-25 13:47 UTC by Suyash Garg
Modified: 2016-11-03 13:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Cleaning artistsview.py to adhere PEP-8 Standards (7.92 KB, patch)
2016-10-25 13:53 UTC, Suyash Garg
none Details | Review
more fixes (10.15 KB, patch)
2016-10-25 23:17 UTC, Marinus Schraal
none Details | Review
Cleaning artistsview.py to adhere PEP-8 Standards (10.45 KB, patch)
2016-10-26 20:54 UTC, Suyash Garg
none Details | Review
Cleaning artistsview.py to adhere PEP-8 Standards (10.62 KB, patch)
2016-10-28 10:57 UTC, Suyash Garg
none Details | Review
Cleaning artistsview.py to adhere PEP-8 Standards (10.66 KB, patch)
2016-10-30 06:20 UTC, Suyash Garg
needs-work Details | Review

Description Suyash Garg 2016-10-25 13:47:19 UTC
Cleaning views/artistsview.py according to PEP-8 standards.

GNOME Music is written in Python and aspires to adhere to the coding style described in PEP-8:
https://www.python.org/dev/peps/pep-0008/

Use of docstrings is recommended following PEP-257:
https://www.python.org/dev/peps/pep-0257/
Comment 1 Suyash Garg 2016-10-25 13:53:37 UTC
Created attachment 338416 [details] [review]
Cleaning artistsview.py to adhere PEP-8 Standards

Gnome-music is written in Python and
aspires to adhere to the coding style described in PEP-8

Changed some variable names from variableName to variable_name
Made some indentation and binary operator rules to adhere PEP-8
standards
Comment 2 Marinus Schraal 2016-10-25 23:17:19 UTC
Created attachment 338463 [details] [review]
more fixes
Comment 3 Marinus Schraal 2016-10-25 23:24:58 UTC
Review of attachment 338416 [details] [review]:

I think a fine first effort, but there's still lots to gain.

I attached a patch that is on top of this one and does my pass on about half of the file, maybe you can finish the class based on it?

::: gnomemusic/views/artistsview.py
@@ +49,3 @@
         self.items_selected_callback = None
+        self.artist_albums_stack = Gtk.Stack(
+                transition_type=Gtk.StackTransitionType.CROSSFADE,)

indent too far (much more of this, not gonna point it out any further)

@@ +163,3 @@
+                'iter': _iter,
+                'albums': [],
+                'widget': None}

this one is just odd

I think the following is much more readable:

self._a[a.casefold()] = {
    'iter': _iter,
    'albums': []
}
Comment 4 Suyash Garg 2016-10-26 20:54:25 UTC
Created attachment 338545 [details] [review]
Cleaning artistsview.py to adhere PEP-8 Standards

Gnome-music is written in Python and
aspires to adhere to the coding style described in PEP-8

Changed some variable names from variableName to variable_name
Made some indentation and binary operations to adhere PEP-8
standards. Made some internal use variable name from
variable_name to _variable_name
Comment 5 Marinus Schraal 2016-10-26 22:37:41 UTC
Review of attachment 338545 [details] [review]:

some last gotchas

::: gnomemusic/views/artistsview.py
@@ +116,2 @@
+        self._last_selection = itr
+        artist = self.model.get_value(itr, 2)

use shorthand, see readme

@@ +120,3 @@
         if widget:
+            if widget.model == self._player.running_playlist('Artist',
+                                                             widget.artist):

I don't like the look of this, why not split it up.
Get the return of the running_playlist in a var and use that to compare.

@@ +139,3 @@
+                                           self.header_bar,
+                                           self.selection_toolbar,
+                                           self.window)

This is a bit of total nitpick, but if pretty much every argument is on a single line then just do it for all of them here.

I know I'm not very consistent on this issue, but I just go with what looks right.

@@ +199,3 @@
+        self._artist_albums_stack.set_sensitive(
+            not self.header_bar._selectionMode)
+        if (self.header_bar._selectionMode is False

Although comparing with 'is False' is not incorrect (it does work), it is considered bad.
It is an object comparison (if 2 objects are the same), not if 2 values are the same.

But since the first statement is either True or False, we should use the 'not' bool negator.

@@ +200,3 @@
+            not self.header_bar._selectionMode)
+        if (self.header_bar._selectionMode is False
+                and grilo.changes_pending['Artists'] is True):

'is True' is unneeded: the return value of the calls is already True

@@ +212,3 @@
         for path in self.view.get_selection():
+            itr = self.model.get_iter(path)
+            artist = self.model.get_value(itr, 2)

Use treemodel shorthands, see the readme and the earlier code.
self.model[itr][2]

@@ +235,3 @@
             else:
+                self._items_selected_callback(self._items_selected)
+

empty line be gone
Comment 6 Suyash Garg 2016-10-28 10:57:59 UTC
Created attachment 338678 [details] [review]
Cleaning artistsview.py to adhere PEP-8 Standards

Gnome-music is written in Python and
aspires to adhere to the coding style described in PEP-8

Changed some variable names from variableName to variable_name
Made some indentation and binary operations to adhere PEP-8
standards. Made some internal use variable name from
variable_name to _variable_name. Also, using treemodel shorthands
and made some changes in object comparisions.
Comment 7 Suyash Garg 2016-10-30 06:20:09 UTC
Created attachment 338788 [details] [review]
Cleaning artistsview.py to adhere PEP-8 Standards

Gnome-music is written in Python and
aspires to adhere to the coding style described in PEP-8

Changed some variable names from variableName to variable_name
Made some indentation and binary operations to adhere PEP-8
standards. Made some internal use variable name from
variable_name to _variable_name. Also, using treemodel shorthands
and made some changes in object comparisions. Some last changes
and removing some newlines.
Comment 8 Marinus Schraal 2016-11-03 12:09:19 UTC
Review of attachment 338788 [details] [review]:

Getting there.

::: gnomemusic/views/artistsview.py
@@ +121,3 @@
+            playing_artist = self._player.running_playlist('Artist',
+                                                            widget.artist)
+            selected_artist = self._artist_albums_stack

I would call them artist_{playing,selected} (artist is playing/selected).

The naming isn't really a representation of what we are getting here, we are retrieving a model and a widget (wildly different things). So either some rewrite is in order or at least make it clear what it is we're doing.

@@ +137,3 @@
+        self._artist_albums_stack.add(new_artist_albums_widget)
+        artist_albums = None
+        artist_albums = ArtistAlbumsWidget(artist,

Why set it to None first and then make a new object.

@@ +166,3 @@
+            itr = self.model.insert_with_valuesv(-1, [2], [artist])
+            self._artists[
+                artist.casefold()] = {

why the extra line for the square brackets?

@@ +178,3 @@
             self.window._init_loading_notification()
+            GLib.idle_add(grilo.populate_artists,
+                          self._offset, self._add_item)

fill out the line as much as possible
Comment 9 Marinus Schraal 2016-11-03 13:04:27 UTC
Ok I used your last patch with some of my comments and some fixes on top of that to push to the tree.

There's more work to be done here, but that involves rewriting bigger parts.

Please do review my further changes and comments above to improve future submissions.

Thanks for your work here.

This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.