> Nice improvement, thanks!  Notes follow:

Thanks for the review!

> 
> src/maasserver/fields.py
> ------------------------
> 
> This follows the pattern as it already was, so not a fault of this branch per
> se, but:
> 
>     mac_error_msg = "'%(value)s' is an invalid MAC address."
> 
> I often forget this, but I think we were supposed to stick to upper-case names
> for globals.
> 
> Could you make the message say “is not a valid MAC address” instead of “is an
> invalid MAC address”?  The only reason to call the value a MAC address at all
> here is that the user, mistakenly, presented it as one.  Echoing a part of
> their incorrect belief back to them while contradicting another part implies
> partial affirmation: “Yes, this is a MAC address like you said, but it's not a
> valid one.”  That could mean “the vendor ID is a reserved value,” or who
> knows, “there's another NIC with the same MAC out there.”  It will all work
> out in the end, but there is room for confusion.
> 
> Of course that does raise questions about the “One or more MAC addresses is
> invalid” elsewhere in the code.  Not as serious, I think, because it's clear
> that the message doesn't go into the technical details.  How about “Malformed
> value in MAC addresses”?

I think I prefer “is not a valid MAC address”.  And I turned mac_error_msg into MAC_ERROR_MSG and mac_re into MAC_RE.

> What is VerboseRegexValidator.code for?  It looks mysterious.  Comment?

Done.

> 
> .
> 
> The docstring for VerboseRegexValidator.__call__ says:
> 
>         Validates that the input matches the regular expression.
> 
> We normally put the first line of docstring right after the opening quotes,
> and if it's a single-line docstring and it will fit, we put the closing quotes
> on the same line.  Did you mean to do some more work on this docstring before
> proposing the branch for merging?  Was it originally written for __init__?
> 
> Also, this is a function docstring — that sentence should be in the
> imperative.

Yeah, I've been lazy, this is a comment I copied straight from Django's code.  Fixed.

> .
> 
> VerboseRegexField.__init__ has this docstring:
> 
>         regex can be either a string or a compiled regular expression object.
>         error_message is an optional error message to use, if
>         'Enter a valid value' is too generic for you.
> 
> That's not very easy to read, for several reasons.  The parameter names make
> it impossible to start the sentences with capitals, so why not use the
> standard :param: fields?  Single non-directional quotes look strange in
> running text, and resemble apostrophes.  The ‘error_message’ parameter is
> actually just called ‘message.’  And, you're explaining ‘message’ in terms of
> an obscure default that is hidden away in Django somewhere, irrelevant to this
> function and, as you point out, uselessly generic.  You did the right thing in
> not supporting the default; now remove it from the docstring!

Same here (comment copied from Django), fixed.

> src/maasserver/forms.py
> -----------------------
> 
> It's a bit odd to have MAX_NUMBER_OF_MESSAGES be so verbose about “number of,”
> even while it abbreviates “maximum.”  Maybe just call it MAX_MESSAGES?

All right, done.

> .
> 
> I like merge_error_messages, and I don't want to slow you down too much on
> this, but manipulating text like this is always tricky.  It's easy to produce
> gibberish; you'll know what I mean if you've ever seen MS-DOS or Windows 95
> boot in a language other than English.
> 
> If you want to concatenate independently produced, stand-alone sentences or
> even brief paragraphs, they need very clear separators that “outrank” any
> punctuation that you expect within the messages.
> 
> As an extreme example, you don't want:
> 
>     “Missing DHCP server.  Enter a local server.”
> 
> plus
> 
>     “IP outside of network range.”
> 
> ...to merge into:
> 
>     “Missing DHCP server.  Enter a local server, IP outside of network range.”
> 
> One way to avoid all problems is to make it clear that this is not a
> completely generic function, that it makes certain assumptions about the error
> strings it gets to deal with, and to make sure those assumptions hold.  But I
> think there's another way.
> 
> Instead of stringing these messages together using commas, I would use spaces
> and emdashes between the messages, and probably even keep the messages
> themselves intact:
> 
>     “Missing DHCP server. Enter a local server. — IP outside of network
> range.”
> 
> Ugly, but hopefully less confusing.  The emdash will look very narrow here if
> you view it in a monospace font, but will come out wider in the browser.  In a
> pinch, if neither non-ASCII characters or HTML entity codes are acceptable,
> you could fake an emdash using two endashes.
> 
> The ellipsis message (nice nomenclature by the way!) needs a separator as
> well.  Otherwise you'll get things like:
> 
>     “Name is too long.  Maximum allowed is 63 characters and 3 more error(s).”
> 
> Compare how many commas English normally requires even in short enumerations
> like “bread, eggs, and butter.”
> 
> My point is not that the code isn't clever enough to deal with every scenario
> I can come up with.  What I mean to convey is that micro-managing text is a
> humongous rabbit hole.  As a rule of thumb, try to leave text intact even if
> it forces you into slightly awkward phrasing.  I won't think less of you for
> running away from the problem.  :-)
> 
> (Pop text-processing quiz: If x is an all-ASCII unicode string, is x.lower()
> guaranteed to be an all-ASCII unicode string?  All code is broken. :-)

Yeah, that's right, it's probably a bad idea to assume too much about the messages.  I've implemented your idea.

> 
> .
> 
> Maybe you could also put each line of the string-interpolation tuple in
> merge_error_messages on a separate line, the way we were expected to do in
> Launchpad?  With all the caveats compressed into such a small piece of code,
> simple as it is, it pays to be extra-clear.

Done.

> 
> .
> 
> Another one of those it-was-this-way-when-you-found-it things: a few free-form
> text strings in this file are single-quoted, which I recommend against because
> we don't want apostrophes to feel unwelcome in free-form text.  Any chance I
> could get you to fix some of them?
> 
> The ones I spotted where in WithMACAddressesMixin.is_valid ('One or more MAC
> addresses is invalid.') and in WithMACAddressesMixin.clean_mac_addresses ('MAC
> address %s already in use.').  Your prior improvement of the latter does not
> go unappreciated though!

Fixed.

> 
> .
> 
> src/maasserver/tests/test_fields.py
> -----------------------------------
> 
> The test_VerboseRegexValidator_validates_value and
> test_VerboseRegexField_validates_value methods are slightly misleadingly
> named: what they test is that well-formed values are accepted.  The validator
> could forget to validate and the tests would still pass.
> 
> .
> 
> src/maasserver/tests/test_forms.py
> ----------------------------------
> 
> Don't forget to test the other edge case for merge_error_messages: exactly 1
> more error.

Right, done.

> .
> 
> In test_merge_error_messages_includes_limited_number_of_msgs, why not pass a
> custom limit?

Because I want to test that there is a default limit.

> 
> .
> 
> If you're still going to strip off punctuation and insert your own, be sure to
> test it thoroughly.  Here is the problem space I'm talking about:
> 
>     {message-ends-without-punctuation, message-ends-in-full-stop, message-
> ends-in-other-punctuation}
>     ×
>     {limit-reached, limit-not-reached}
> 
> As far as I can see, the combination (message-ends-in-other-punctuation,
> limit-not-reached) produces bad punctuation.  An error message “Are you root?”
> with no further messages would show up as “(Are you root?.)”
> 
> On the other hand of course, I won't blame merge_error_messages if it screws
> up error messages like
> 
>     “Sentence must end in .”
> 
> ...and I would support any invective you would care to hurl at whoever wrote
> such a message.  :-)