GNOME Bugzilla – Bug 772720
[cleanup] artistalbumwidget
Last modified: 2017-02-24 10:35:33 UTC
Cleaning, removing redundancy and maintaining docstrings in artistalbumwidget.py 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/
Created attachment 337378 [details] [review] artistalbumwidget: Clean out unused playlists import 'Playlists' was never used and was no longer required. This patch removes the unused playlists import.
Review of attachment 337378 [details] [review]: lgtm You can keep the description more terse here, it basically sais the same thing three times. Just the header would do.
Created attachment 337464 [details] [review] artistalbumwidget: Clean out unused playlists import
@Marinus: Yes. Even I thought so, but leaving the description empty seemed somewhat odd to me. Updated it now.
Created attachment 337482 [details] [review] artistalbumwidget: Format code to follow pep8 style Remove unrequired backslash and make sure code breaks before binary operator('%' in this case).
Created attachment 337516 [details] [review] pep8 fixes
Review of attachment 337516 [details] [review]: An example of things I would fix in this class. Note that I just did 1 change each as example, so there's a lot more to cleanup. ::: gnomemusic/widgets/artistalbumwidget.py @@ -58,2 @@ @log - def __init__(self, artist, album, player, model, header_bar, selectionModeAllowed): pep8 line length @@ -73,3 @@ self.header_bar = header_bar - self.selectionMode = False - self.selectionModeAllowed = selectionModeAllowed no camelCase local class variables prepend with _ @@ -88,3 @@ - '<span color=\'grey\'>(%s)</span>' - % str(album.get_creation_date().get_year()) - ) line length simplify by breaking apart do the get_creation_date() call only once use .format instead of % (preferred - not really pep8) closing brackets on the same line for function calls @@ -95,2 @@ @log - def add_item(self, source, prefs, track, remaining, data=None): for internal class functions prepend with _ @@ -121,3 @@ track.song_widget = song_widget itr = self.model.append(None) - song_widget._iter = itr An objects variable by nature should not be prepended with _ if called from another class iter is a keyword in python so itr could also be fine, for clarity @@ -131,3 @@ - self.model.set(itr, - [0, 1, 2, 3, 5], - [title, '', '', False, track]) Use short-style treemodel manipulation if possible (see README) @@ -158,3 @@ - if not self.selectionMode and \ - (event.button == Gdk.BUTTON_SECONDARY or - (event.button == 1 and event.state & Gdk.ModifierType.CONTROL_MASK)): Ok this statement is really too big, so needs more work. But the idea is to give every boolean statement its own line with proper indentation. prepend per line with the boolean operator @@ -165,3 @@ - if self.selectionMode: - self.model[widget._iter][6] = not self.model[widget._iter][6] iter here is the song_widget as above, so might as well name it the same for clarity sake @@ -170,3 @@ self.player.stop() - self.player.set_playlist('Artist', self.artist, - widget.model, widget._iter, 5, 6) line length @@ -192,3 @@ - if not self.selectionMode: - return - if not model[_iter][5]: this can be shorter, not really pep8 but a general cleanup also a simpler example of multi-line boolean if statements
Review of attachment 337482 [details] [review]: Did you even use the pep8 tool? There's a lot more than this.
Review of attachment 337464 [details] [review]: lgtm
Created attachment 338050 [details] [review] artistalbumwidget: pep8 fixes change camelCase to camel_case wherever possible. use .format instead of % reduce line length to pep8 standards by breaking apart closing brackets on the same line for function calls NOTE: This is just an interim patch and not the final one, just to make sure that I'm proceeding in the right direction.
Created attachment 338051 [details] [review] artistalbumwidget: pep8 fixes change camelCase to camel_case wherever possible. use .format instead of % reduce line length to pep8 standards by breaking apart closing brackets on the same line for function calls NOTE: This is just an interim patch and not the final one, just to make sure that I'm proceeding in the right direction. https://bugzilla.gnome.org/show_bug.cgi?id=772720 change camelCase to camel_case wherever possible. use .format instead of % reduce line length to pep8 standards by breaking apart closing brackets on the same line for function calls NOTE: This is just an interim patch and not the final one, just to make sure that I'm proceeding in the right direction.
@Marinus: I am still getting confused on what all to prepend with _. If you can fix up a whole function as an example that will be great. And I used 'pep8online.com' checker tool. Do you think it's good or you prefer another? And sorry for the double commit message , messed it up while amending my commit (:P).
Review of attachment 338051 [details] [review]: some minor nitpicks ::: gnomemusic/widgets/artistalbumwidget.py @@ +94,2 @@ self.ui.get_object('year').set_markup( + '<span color=\'grey\'>({})</span>'.format(str(year))) the str() is unneeded here @@ +145,3 @@ @log def _update_album_art(self): + print(self.album) i added to my commit by mistake @@ +191,3 @@ + def _check_button_toggled(self, button, song_widget): + if song_widget.model[song_widget.iter][6] != button.get_active(): + song_widget.model[song_widget.iter][6] = button.get_active() make a var out of bttn.get_active
Created attachment 339270 [details] [review] artistalbumwidget: pep8 fixes change camelCase to camel_case wherever possible use .format instead of % reduce line length to pep8 standards by breaking apart closing brackets on the same line for function calls remove unrequired str() remove unused print statement assigned var to a function that was called frequently
Thanks for the work here. As explained back then I sort of happened to rewrite most of this class and made the new code conform with pep-8. So this got fixed.