GNOME Bugzilla – Bug 786541
Missing loggers from 2 view modules.
Last modified: 2017-08-21 21:59:06 UTC
Created attachment 358016 [details] [review] Imports logging and Initiates a logger object for each module. There were logger objects on files artistview.py and songsview.py without a logger instance. If they were ever called they probably crashed the whole program. This patch imports logging and initiates a "logger = logging.getLogger(__name__)" object on each module.
Review of attachment 358016 [details] [review]: For commit message see https://wiki.gnome.org/Git/CommitMessages in general and see earlier commits for how the project likes it. Keep it short and to the point: eg. 'Add missing logger imports' is sufficient here. Try using git-bz for attaching patches : https://wiki.gnome.org/Git/WorkingWithPatches#How_to_attach_a_patch_to_a_bug_in_Bugzilla ::: gnomemusic/views/artistsview.py @@ +23,3 @@ # delete this exception statement from your version. +import logging blank line after this @@ +132,3 @@ if widget: artist_widget_model = self.player.running_playlist('Artist', + widget.artist) Whitespace corrections are fine, but should be a separate patch. ::: gnomemusic/views/songsview.py @@ +23,3 @@ # delete this exception statement from your version. +import logging blank line @@ +91,3 @@ try: itr = self.model.get_iter(path) + except ValueError as err: separate patch
Created attachment 358017 [details] [review] Removed extra blank line, redid the commit message. I redid the commit message, hope its better and clearer now. I am sorry for the blank lines, my editor has autopep8 enabled by default and didn't checked the diff before.
Review of attachment 358017 [details] [review]: The links I gave are just a guideline, as I said a commit like this just needs some minor clarification. This is too much, the patch itself is clear enough. 'Add missing logger import' or something like it is fine for this. Don't go overboard. Also the 'title' (1st) line of the commit msg should not exceed 50 chars if possible (see the link about commit messages). If the 1st line is enough information, don't go make up a story below it.
Created attachment 358054 [details] [review] Short and to the point I hope oh g, about the commit message, when I read the review, I read insufficient for some reason, probably cause of anxiety from first time contributing. Anyway that should do it.
Review of attachment 358054 [details] [review]: Looks good to me.
Thanks for the patch.