Code review comment for lp:~danilo/launchpad/bug728370-nondirect-subs

Revision history for this message
Gary Poster (gary) wrote :

This looks great, Danilo!

When reading the tests, I kept wanting to try and reduce the copy and paste elements of the test code by suggesting some helper functions. Maybe they exist, but when I compared the various "test_team_member_multiple" tests in the file as an example, they were not as similar as I expected. Therefore, I share the vague concern with you in case you have any ideas, but I don't ask you to make any changes because I didn't see any good ones after a quick look.

I had a couple of very small items. I already brought up the fact that the "short (just-enough)" parts of your test suite descriptions caused me to be a bit confused, for little gain. No biggie. I also wondered about your switch/case indentation: I would have indented your case statements. Maybe you have a good reason for them to be the way they are. Case statements are used infrequently enough that I doubt we have a rule for them. If you like it the way it is, that's fine.

I was going to ask about how you were going to use your strings--using Y.substitute and Y.Escape.html and so on--but you beat me to the bunch by bringing up the question in another forum, so nevermind. :-)

Thank you!

Gary

« Back to merge proposal