Code review comment for lp:~jcsackett/launchpad/dont-expose-domains

> The conversion from doctest to unittest looks like it could use some
> more fleshing out.
>
> Two of the scenarios from the doctest that look important aren't
> represented in the unittests:
>
> >>> generate_nick("<email address hidden>")
> 'foo-bar'
> >>> generate_nick("<email address hidden>")
> 'foo+bar'

Fair, I've updated the symbol handling test to show several versions.

> The second half of the doctest (starting with "It should be nearly
> impossible to..." and going through the testing of
> NicknameGenerationError) looks important too and those tests aren't
> represented in the unittests.

They're not anymore; that part tests the use of email domains as part of what's being used for nick generation to the point where random prefixes and suffixes start being used. Now we skip straight to the randomized stuff, and that's tested in the non-colliding part.

> Here are some smaller things that occurred to me during the review:
>
> The regex in lib/lp/registry/model/person.py doesn't need the second set
> of capturing parens now that we're not using the domain name.

Correct, I've updated the second group to noncapturing.

> These two lines from the end of test_can_create_noncolliding_nicknames
> appear to be copypasta (and therefore can be removed):
>
> self._useNicknames(['bar-c'])
> self.assertEqual('bar-c', nick)

Actually, that was supposed to be a test to show what happened if it got another bar@ email address after one collision. It's been updated.

> It would be nice if test_enforces_minimum_length asserts that "i" isn't
> registered at test start to ensure that we're really seeing
> nick-too-short behavior and not nick-already-taken behavior.

Done.