openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

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.