Some feedback on these patches:
It's good to separate style fixes into separate commits. I'm glad you fixed my
mis-spelling of the word "quotation", but it's easier to review if that is its
own separate patch.
In general, it's usually pretty easy for me to review a series of patches; try
to ensure that each one is somehow a coherent change.
git on my computer complains about some lines of yours that have a " " character
(space) as the last character on the line.
I didn't realize that one could use the | character to combine QuerySet objects
like that. That's awesome.
Your test is also of good quality! I'm glad you test the case-insensitivity. I
generally prefer to make assertions about the contents of a list, but since you
have a QuerySet, it's a little harder. For that reason, you could convert it to
a list like:
self.assertEqual([User.objects.get(username='spinoza')], list(atq_filter_spinoza))
The way you have it is fine, so I'm not going to change it.
Also, you have what seems to be a debugging print left commented-out at the top
of the
PeopleFinderTagQueryTests.test_all_tags_insensitive_case_search_multiple_names
method. I'll remove that, although it's a great thing to have while testing. I
also like to do "import pdb; pdb.set_trace()" to break into a debugger when
running my own tests.
I've pushed this, and will deploy it once it passes the Hudson automated testing
at http://linode2.openhatch.org:8080/ . Yay -- marking as 'resolved'!
|