Review: Needs Fixing Hi Oli,
I'm just looking at your recent rename, and it doesn't quite make sense to me. If the variable is called "verse_number_display" it makes me think this tells me how the verse number is to be displayed. However, since the variable is attached to a checkbox, I realised that it is actually whether the verse number should be hidden or visible. Might I suggest renaming the variable to: is_verse_number_visible self.is_verse_number_visible = True # You can see the verse numbers self.is_verse_number_visible = False # Now you can't see them Then you should rename the checkbox too: self.is_verse_number_visible_check_box = QtGui.QCheckBox() Now, on to the tests. Firstly, I want to say that I am very impressed with your first attempt at a test. This looks really good, and it looks like you're catching on to how to write tests pretty quickly. Keep on! Just a few minor details (mostly style): - Line 138 does not need to be blank. You can remove it. - Please write a docstring for the TestVerseReferenceList class. - Line 146: "add_test(self)" is not a well named method. Rather name it "add_verse_reference_test" - Line 150 does not need to be blank. You can remove it. - You should never have more than 1 GIVEN, WHEN, THEN set in your test. If you have more than one, break your test up into more tests. You can start the next test with a GIVEN: 1 line in the list of verses. - There should be a space between the # and GIVEN/WHEN/THEN e.g. # GIVEN: An empty verse list - There should be spaces either side of a %, e.g. "%s" % book - If you're adding items to a list of some sort, I highly recommend checking len(list) - Once again, extra blank lines are not necessary. Please remove lines 200, 208 and 218. - Even in tests, variable names should be PEP8 compliant: old_len, not oldLen. - Don't call __len__(), use len(self.reference_list.version_list) -- https://code.launchpad.net/~oliwee/openlp/HideBibleVerses/+merge/180651 Your team OpenLP Core is subscribed to branch lp:openlp. _______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : [email protected] Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
