GNOME Bugzilla – Bug 777571
Change required for searchbar.py according to PEP-8 standards.
Last modified: 2017-12-29 15:26:58 UTC
Created attachment 343946 [details] The file I have attached is searchbar.py whose path under jhbuild is - /home/dor/jhbuild/install/lib/python3.5/site-packages/gnomemusic/. The file named searchbar.py is not cleaned up according to PEP-8 standards. Especially, it is not following the rule for 79 characters in a line.
Sorry for the delay. We really need a diff here. If you use the git checkout of the source tree (which you should), you should be able to make one. I'd suggest you look up some guides on this, https://wiki.gnome.org/Git/Developers is one source focused on working with the gnome git tree specifically, see the part about 'contributing patches'.
Alright! I am going to have a look there.
Created attachment 345046 [details] [review] Modification according to PEP-8 standards. I have uploaded the patch, which shows the diff. Please have a look!
Review of attachment 345046 [details] [review]: Thanks for the diff. Review has a lot of points, but mostly similar things. Remember anything internal to the class should be prepended with an underscore: _function, self._var, etc. All forms of camelCase internal to the class should be removed (functionName -> function_name). If you could work on this we can take it from there. ::: gnomemusic/searchbar.py @@ +83,3 @@ + selected_value = [x for x in self.values + if x[BaseModelColumns.ID] == selected_id + ] I think i would close bracket on the 2nd line in this case, @@ +91,3 @@ # No need to set the tag there if (selected_value[BaseModelColumns.ID] != 'search_all' and + selected_value[BaseModelColumns.ID] != 'grl-tracker-source'): if (a and b): operator on the start of the new line with indentation. @@ +93,3 @@ + selected_value[BaseModelColumns.ID] != 'grl-tracker-source'): + self.tag.set_label(selected_value[BaseModelColumns.NAME]) + self.entry.add_tag(self.tag) these should be just 1 indent further than the if (like it was before). @@ +96,2 @@ else: + self.entry.remove_tag(self.tag) I don't see what changed here. @@ +168,3 @@ + weight=Pango.Weight.BOLD, + weight_set=True + ) For arguments I prefer end-of-line style like (depending if it fits on line): function(arg1, arg2) function(arg1, arg2) function( long_arg1, long_arg2) @@ +174,3 @@ + 'text', + BaseModelColumns.HEADING_TEXT + ) same @@ +179,3 @@ + self._visibilityForHeading, + True + ) same as above @@ +209,3 @@ @log def _row_activated(self, view, path, col): + id = self.model.get_value(self.model.get_iter(path), BaseModelColumns.ID) shorthands for tree models self.model[self.model.get_iter(path)][BaseModelColumns.ID] @@ +220,2 @@ @log + def _visibilityForHeading(self, col, cell, model, _iter, additional_arguments): _visibility_for_heading (no more camelCase) better maybe _header_visibility ? @@ +227,3 @@ + cell.set_visible(visible == ( + model[_iter][BaseModelColumns.HEADING_TEXT] != "") + ) closing parenthesis on 2nd line @@ +243,3 @@ + halign=Gtk.Align.CENTER, + valign=Gtk.Align.START + ) read above about function arguments style I'm not gonna comment on it below, but it should be corrected everywhere. @@ +260,3 @@ + _("Sources"), + searchbar._search_entry + ) no camelCase self.sources_manager however, should also be internal i guess so self._sources_manager @@ +263,1 @@ self.sourcesFilter = FilterView(self.sourcesManager, self) same as above @@ +271,3 @@ + _("Match"), + searchbar._search_entry + ) all of the above @@ +285,3 @@ manager.set_active(id) if manager == self.sourcesManager: + self.searchFieldsFilter.view.set_sensitive(id == 'grl-tracker-source') idem @@ +306,1 @@ self._searchContainer.get_style_context().add_class('linked') _search_container @@ +382,3 @@ @log def toggle_bar(self): +self.show_bar(not self.get_search_mode()) this is not right and won't even execute
Created attachment 345941 [details] [review] Did more work on the review to improve the code.
Review of attachment 345941 [details] [review]: Please look at recent examples of PEP-8 updates in the project history (git log) to see how things are done, those are the best way to pick up what we would like to see. ::: gnomemusic/searchbar.py @@ +65,3 @@ ['search_album', _("Album"), ''], ['search_composer', _("Composer"), ''], + ['search_track', _("Track Title"), ''],] This is about the only place I actually think the old style was ok. This is creating a list, not function arguments as the other places were. So closing bracket on the next line is ok. @@ +84,3 @@ if selected_value != []: selected_value = selected_value[0] self.selected_id = selected_value[BaseModelColumns.ID] self.selected_id -> self._selected_id @@ +123,3 @@ value = [source.get_id(), source.get_name(), ''] + _iter = self._model.append() + self._model.set(_iter, [0, 1, 2], value) treeviewmodel shorthands self._model[_iter][0, 1, 2] = value @@ +148,3 @@ GObject.TYPE_STRING, # ID GObject.TYPE_STRING, # NAME + GObject.TYPE_STRING, # TEXT]) can this be lined out behind the initial ( ? @@ +163,3 @@ + self._rendererHeading = Gtk.CellRendererText(weight=Pango.Weight.BOLD, + weight_set=True) line out to the opening ( @@ +166,3 @@ col.pack_start(self._rendererHeading, False) col.add_attribute( + self._rendererHeading,'text',BaseModelColumns.HEADING_TEXT) line out to the opening ( @@ +169,2 @@ col.set_cell_data_func( + self._rendererHeading,self._header_visibility,True) idem @@ +172,2 @@ self._rendererRadio = Gtk.CellRendererToggle( + radio=True,mode=Gtk.CellRendererMode.INERT) idem and everything following @@ +189,3 @@ @log def _row_activated(self, view, path, col): + id = self._model.get_value(self._model.get_iter(path), BaseModelColumns.ID) shorthand id = self._model[iter][id] @@ +232,3 @@ def initialize_filters(self, searchbar): + self._sources_manager = SourceManager( + 'source',_("Sources"),searchbar._search_entry) space behind , also line out on opening ( @@ +240,3 @@ + self._search_fields_manager = BaseManager( + 'search',_("Match"),searchbar._search_entry) spaces @@ +269,3 @@ + self._dropdown = dropdown + self._search_container = Gtk.Box( + orientation=Gtk.Orientation.HORIZONTAL,halign=Gtk.Align.CENTER) spaces and ditto below
Created attachment 347770 [details] [review] Code Clean-up, following PEP-8 standards. More improvement done, including treeview model shorthands and included proper spacing.
(In reply to Rashi from comment #7) > Created attachment 347770 [details] [review] [review] > Code Clean-up, following PEP-8 standards. > > More improvement done, including treeview model shorthands and included > proper spacing. Can you rebase to patch to master? I'm getting a sha1 error probably b/c you did this work on some local branch and I can't apply it with git bz.
Yeah sure! Can you please tell me how do I rebase the patch to master, Just steps i want to know :)
(In reply to Rashi Sah from comment #9) > Yeah sure! > Can you please tell me how do I rebase the patch to master, Just steps i > want to know :) This is information that is available online: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow has some bits about it. But there's plenty (better) in-depth guides around.
Created attachment 349189 [details] [review] searchbar: More improvement according to PEP-8 standards.
Thanks for working on this issue. Searchbar.py has had some rework done in between and I had to start this from scratch, using your work as reference. Check commit 41a558980958ed7ec3cbae4518d9f2bbb26f9ea1 to see what has been done. 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.