GNOME Bugzilla – Bug 773769
Cleaning songsview.py according to PEP-8 Standards
Last modified: 2016-11-21 14:23:26 UTC
Clean songsview.py to make the code more close to PEP-8 Standards. As for the goals of release 3.24, it is desired to clean up the codebase and make it more unified, using PEP8 and PEP257. 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 338884 [details] [review] Clean songsview.py to make code close to PEP-8. As for release 3.24 it is required to clean the codebase according to the PEP8 and PEP257. Cleanup of songsview.py. Removed unused import of ArtSize from albumartcache Using shorthands for ListStore model Done indentation acc to PEP8 made use of not in if-else statements instead of comparison with False changed some variableNames to variable_names
Review of attachment 338884 [details] [review]: Getting there. This is ofc only a review of what I can see in the patch. I noticed at least some 'if' statements in the file that could be optimized. ::: gnomemusic/views/songsview.py @@ +41,3 @@ def __init__(self, window, player): + BaseView.__init__(self, 'songs', _("Songs"), + window, Gd.MainViewType.LIST) In this case I would only place Gd.MainViewType on the next line aligned to the ( @@ +49,3 @@ .add_class('songs-list') + self._icon_height = 32 + self._icon_width = 32 these aren't used anymore @@ +121,3 @@ [utils.get_media_title(item), + artist, item, bool(item.get_lyrics())] + ) I would fill out the 2nd square bracket line as much as possible. @@ +159,3 @@ xpad=32, xalign=1.0 ) I don't see a point going 4 lines for this, function style multiline should just use the same rules as other stuff and not have a closing parenthesis the next line imo. Same goes for title_renderer. It just breaks completely with all other style there is. @@ +204,3 @@ minutes = seconds // 60 seconds %= 60 cell.set_property('text', '%i:%02i' % (minutes, seconds)) theres an utils function for this too now (time to hh:mm) @@ +213,2 @@ if item: cell.set_property('text', item.get_album() or _("Unknown Album")) get_album should use the utils.py function, oversight on my part.
Created attachment 339102 [details] [review] songsview: Cleanup As for release 3.24 it is required to clean the codebase according to the PEP8 and PEP257. Adhere to PEP-8 and PEP-257: * stop using CamelCase for variables and functions * indentation fixes * mark variables private * use treemodel shorthands * use docstrings * use of some utils functions * other cleanups
Review of attachment 339102 [details] [review]: minor nitpicks, other than that lgtm ::: gnomemusic/views/songsview.py @@ +100,3 @@ + param player: The main player object + param playlist: The current playlist object + param current_iter: Iter of the current displayed tracks : missing in front @@ +236,3 @@ + In this view this will be the all the songs selected + : returns: All selected songs + : rtype: A list of tracks no space after :
Created attachment 339162 [details] [review] songsview: Cleanup As for release 3.24 it is required to clean the codebase according to the PEP8 and PEP257. Adhere to PEP-8 and PEP-257: * stop using CamelCase for variables and functions * indentation fixes * mark variables private * use treemodel shorthands * use docstrings * use of some utils functions * other cleanups
Did some final touch-ups (please do check) and pushed this. Thanks for your work.