Merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:~maas-committers/maas/trunk
- form-error-msg-bug-1308292
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Raphaël Badin |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2284 |
Proposed branch: | lp:~rvb/maas/form-error-msg-bug-1308292 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
360 lines (+159/-24) 8 files modified
src/maasserver/api.py (+2/-2) src/maasserver/fields.py (+43/-9) src/maasserver/forms.py (+34/-5) src/maasserver/tests/test_api_enlistment.py (+5/-2) src/maasserver/tests/test_api_macaddress.py (+1/-1) src/maasserver/tests/test_api_node.py (+3/-3) src/maasserver/tests/test_fields.py (+31/-0) src/maasserver/tests/test_forms.py (+40/-2) |
To merge this branch: | bzr merge lp:~rvb/maas/form-error-msg-bug-1308292 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+216416@code.launchpad.net |
Commit message
Make NodeWithMACAddr
Description of the change
I had to create custom VerboseRegexVal
Raphaël Badin (rvb) wrote : | # |
> Nice improvement, thanks! Notes follow:
Thanks for the review!
>
> src/maasserver/
> -------
>
> 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 VerboseRegexVal
Done.
>
> .
>
> The docstring for VerboseRegexVal
>
> 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.
> .
>
> VerboseRegexFie
>
> 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/
> -------
>
> It's a bit odd to have MAX_NUMBER_
> even while it abbreviates “maximum.” Maybe just call i...
Raphaël Badin (rvb) wrote : | # |
> (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. :-)
Looks like it, what am I missing? http://
Jeroen T. Vermeulen (jtv) wrote : | # |
The Turkish language has two letters “i”: with the dot and without. As I understand it, and it looks very sensible to me, the lower-case version of ‘I’ is ‘ı’ whereas the upper-case version of ‘i’ is ‘İ’.
So presumably, with a Turkish locale, "HI".lower() == "hı"!
Raphaël Badin (rvb) wrote : | # |
> The Turkish language has two letters “i”: with the dot and without. As I
> understand it, and it looks very sensible to me, the lower-case version of ‘I’
> is ‘ı’ whereas the upper-case version of ‘i’ is ‘İ’.
>
> So presumably, with a Turkish locale, "HI".lower() == "hı"!
Jeroen T. Vermeulen (jtv) wrote : | # |
You didn't actually print 'hI'.lower(), just 'hI'.
But yes, it looks like Python doesn't do Turkish upper/lower-casing! Try it using e.g. ‘awk’ and you'll get different results... All software is still broken. :-)
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Ign http://
Ign http://
Hit http://
Get:3 http://
Hit http://
Get:4 http://
Get:5 http://
Get:6 http://
Hit http://
Get:7 http://
Hit http://
Get:8 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Fetched 137 kB in 0s (669 kB/s)
Reading package lists...
sudo DEBIAN_
--
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Hit http://
Ign http://
Ign http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Hit http://
Ign http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Ign http://
Fetched 933 B in 0s (5,962 B/s)
Reading package lists...
sudo DEBIAN_
--
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.
Ign http://
Hit http://
Ign http://
Hit http://
Ign http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Hit http://
Ign http://
Hit http://
Ign http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Reading package lists...
sudo DEBIAN_
--
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.
Ign http://
Ign http://
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Get:3 http://
Hit http://
Get:4 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Hit http://
Hit http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Ign http://
Ign http://
Ign http://
Fetched 137 kB in 0s (712 kB/s)
Reading package lists...
sudo DEBIAN_
--
Preview Diff
1 | === modified file 'src/maasserver/api.py' |
2 | --- src/maasserver/api.py 2014-04-14 23:32:50 +0000 |
3 | +++ src/maasserver/api.py 2014-04-23 07:31:18 +0000 |
4 | @@ -172,7 +172,7 @@ |
5 | Unauthorized, |
6 | ) |
7 | from maasserver.fields import ( |
8 | - mac_re, |
9 | + MAC_RE, |
10 | validate_mac, |
11 | ) |
12 | from maasserver.forms import ( |
13 | @@ -785,7 +785,7 @@ |
14 | match_macs = get_optional_list(request.GET, 'mac_address') |
15 | if match_macs is not None: |
16 | invalid_macs = [ |
17 | - mac for mac in match_macs if mac_re.match(mac) is None] |
18 | + mac for mac in match_macs if MAC_RE.match(mac) is None] |
19 | if len(invalid_macs) != 0: |
20 | raise ValidationError( |
21 | "Invalid MAC address(es): %s" % ", ".join(invalid_macs)) |
22 | |
23 | === modified file 'src/maasserver/fields.py' |
24 | --- src/maasserver/fields.py 2014-02-21 06:59:04 +0000 |
25 | +++ src/maasserver/fields.py 2014-04-23 07:31:18 +0000 |
26 | @@ -17,6 +17,7 @@ |
27 | "MACAddressField", |
28 | "MACAddressFormField", |
29 | "register_mac_type", |
30 | + "VerboseRegexValidator", |
31 | ] |
32 | |
33 | from copy import deepcopy |
34 | @@ -33,20 +34,40 @@ |
35 | SubfieldBase, |
36 | ) |
37 | from django.forms import ( |
38 | + CharField, |
39 | ModelChoiceField, |
40 | - RegexField, |
41 | ) |
42 | +from django.utils.encoding import force_text |
43 | from maasserver.utils.orm import get_one |
44 | import psycopg2.extensions |
45 | from south.modelsinspector import add_introspection_rules |
46 | |
47 | |
48 | -mac_re = re.compile(r'^\s*([0-9a-fA-F]{2}[:-]){5}[0-9a-fA-F]{2}\s*$') |
49 | - |
50 | - |
51 | -mac_error_msg = "Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)." |
52 | - |
53 | -mac_validator = RegexValidator(regex=mac_re, message=mac_error_msg) |
54 | +MAC_RE = re.compile(r'^\s*([0-9a-fA-F]{2}[:-]){5}[0-9a-fA-F]{2}\s*$') |
55 | + |
56 | + |
57 | +MAC_ERROR_MSG = "'%(value)s' is not a valid MAC address." |
58 | + |
59 | + |
60 | +class VerboseRegexValidator(RegexValidator): |
61 | + """A verbose `RegexValidator`. |
62 | + |
63 | + This `RegexValidator` includes the checked value in the rendered error |
64 | + message when the validation fails. |
65 | + """ |
66 | + # Set a bugus code to circumvent Django's attempt to re-interpret a |
67 | + # validator's error message using the field's message it is attached |
68 | + # to. |
69 | + code = 'bogus-code' |
70 | + |
71 | + def __call__(self, value): |
72 | + """Validates that the input matches the regular expression.""" |
73 | + if not self.regex.search(force_text(value)): |
74 | + raise ValidationError( |
75 | + self.message % {'value': value}, code=self.code) |
76 | + |
77 | + |
78 | +mac_validator = VerboseRegexValidator(regex=MAC_RE, message=MAC_ERROR_MSG) |
79 | |
80 | |
81 | def validate_mac(value): |
82 | @@ -157,12 +178,25 @@ |
83 | return nodegroup |
84 | |
85 | |
86 | -class MACAddressFormField(RegexField): |
87 | +class VerboseRegexField(CharField): |
88 | + |
89 | + def __init__(self, regex, message, *args, **kwargs): |
90 | + """A field that validates its value with a regular expression. |
91 | + |
92 | + :param regex: Either a string or a compiled regular expression object. |
93 | + :param message: Error message to use when the validation fails. |
94 | + """ |
95 | + super(VerboseRegexField, self).__init__(*args, **kwargs) |
96 | + self.validators.append( |
97 | + VerboseRegexValidator(regex=regex, message=message)) |
98 | + |
99 | + |
100 | +class MACAddressFormField(VerboseRegexField): |
101 | """Form field type: MAC address.""" |
102 | |
103 | def __init__(self, *args, **kwargs): |
104 | super(MACAddressFormField, self).__init__( |
105 | - regex=mac_re, error_message=mac_error_msg, *args, **kwargs) |
106 | + regex=MAC_RE, message=MAC_ERROR_MSG, *args, **kwargs) |
107 | |
108 | |
109 | class MACAddressField(Field): |
110 | |
111 | === modified file 'src/maasserver/forms.py' |
112 | --- src/maasserver/forms.py 2014-04-10 20:21:24 +0000 |
113 | +++ src/maasserver/forms.py 2014-04-23 07:31:18 +0000 |
114 | @@ -532,6 +532,33 @@ |
115 | |
116 | IP_BASED_HOSTNAME_REGEXP = re.compile('\d{1,3}-\d{1,3}-\d{1,3}-\d{1,3}') |
117 | |
118 | +MAX_MESSAGES = 10 |
119 | + |
120 | + |
121 | +def merge_error_messages(summary, errors, limit=MAX_MESSAGES): |
122 | + """Merge a collection of errors into a summary message of limited size. |
123 | + |
124 | + :param summary: The message summarizing the error. |
125 | + :type summary: unicode |
126 | + :param errors: The list of errors to merge. |
127 | + :type errors: iterable |
128 | + :param limit: The maximum number of individual error messages to include in |
129 | + the summary error message. |
130 | + :type limit: int |
131 | + """ |
132 | + ellipsis_msg = '' |
133 | + if len(errors) > limit: |
134 | + nb_errors = len(errors) - limit |
135 | + ellipsis_msg = ( |
136 | + " and %d more error%s" % ( |
137 | + nb_errors, |
138 | + 's' if nb_errors > 1 else '')) |
139 | + return "%s (%s%s)" % ( |
140 | + summary, |
141 | + ' \u2014 '.join(errors[:limit]), |
142 | + ellipsis_msg |
143 | + ) |
144 | + |
145 | |
146 | class WithMACAddressesMixin: |
147 | """A form mixin which dynamically adds a MultipleMACAddressField to the |
148 | @@ -558,17 +585,19 @@ |
149 | self.errors.get('mac_addresses', None) is not None and |
150 | len(self.data['mac_addresses']) > 1) |
151 | if reformat_mac_address_error: |
152 | - self.errors['mac_addresses'] = ( |
153 | - ['One or more MAC addresses is invalid.']) |
154 | + self.errors['mac_addresses'] = [merge_error_messages( |
155 | + "One or more MAC addresses is invalid.", |
156 | + self.errors['mac_addresses'])] |
157 | return valid |
158 | |
159 | def clean_mac_addresses(self): |
160 | data = self.cleaned_data['mac_addresses'] |
161 | + errors = [] |
162 | for mac in data: |
163 | if MACAddress.objects.filter(mac_address=mac.lower()).exists(): |
164 | - raise ValidationError( |
165 | - {'mac_addresses': [ |
166 | - 'Mac address %s already in use.' % mac]}) |
167 | + errors.append('MAC address %s already in use.' % mac) |
168 | + if errors: |
169 | + raise ValidationError({'mac_addresses': errors}) |
170 | return data |
171 | |
172 | def save(self): |
173 | |
174 | === modified file 'src/maasserver/tests/test_api_enlistment.py' |
175 | --- src/maasserver/tests/test_api_enlistment.py 2014-03-27 04:15:45 +0000 |
176 | +++ src/maasserver/tests/test_api_enlistment.py 2014-04-23 07:31:18 +0000 |
177 | @@ -280,7 +280,7 @@ |
178 | self.assertEqual(httplib.BAD_REQUEST, response.status_code) |
179 | self.assertIn('application/json', response['Content-Type']) |
180 | self.assertEqual( |
181 | - ["Mac address %s already in use." % mac], |
182 | + ["MAC address %s already in use." % mac], |
183 | parsed_result['mac_addresses']) |
184 | |
185 | def test_POST_fails_with_bad_operation(self): |
186 | @@ -316,7 +316,10 @@ |
187 | self.assertEqual(httplib.BAD_REQUEST, response.status_code) |
188 | self.assertIn('application/json', response['Content-Type']) |
189 | self.assertEqual( |
190 | - ["One or more MAC addresses is invalid."], |
191 | + [ |
192 | + "One or more MAC addresses is invalid. " |
193 | + "('invalid' is not a valid MAC address.)" |
194 | + ], |
195 | parsed_result['mac_addresses']) |
196 | |
197 | def test_POST_invalid_architecture_returns_bad_request(self): |
198 | |
199 | === modified file 'src/maasserver/tests/test_api_macaddress.py' |
200 | --- src/maasserver/tests/test_api_macaddress.py 2014-02-20 10:29:15 +0000 |
201 | +++ src/maasserver/tests/test_api_macaddress.py 2014-04-23 07:31:18 +0000 |
202 | @@ -120,7 +120,7 @@ |
203 | self.assertEqual(400, response.status_code) |
204 | self.assertEqual(['mac_address'], list(parsed_result)) |
205 | self.assertEqual( |
206 | - ["Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)."], |
207 | + ["'invalid-mac' is not a valid MAC address."], |
208 | parsed_result['mac_address']) |
209 | |
210 | def test_macs_DELETE_mac(self): |
211 | |
212 | === modified file 'src/maasserver/tests/test_api_node.py' |
213 | --- src/maasserver/tests/test_api_node.py 2014-04-03 16:12:28 +0000 |
214 | +++ src/maasserver/tests/test_api_node.py 2014-04-23 07:31:18 +0000 |
215 | @@ -29,7 +29,7 @@ |
216 | ) |
217 | from maasserver.fields import ( |
218 | MAC, |
219 | - mac_error_msg, |
220 | + MAC_ERROR_MSG, |
221 | ) |
222 | from maasserver.models import Node |
223 | from maasserver.testing import ( |
224 | @@ -574,11 +574,11 @@ |
225 | response = self.client_put( |
226 | self.get_node_uri(node), |
227 | {'power_parameters_mac_address': new_power_address}) |
228 | - |
229 | + error_msg = MAC_ERROR_MSG % {'value': new_power_address} |
230 | self.assertEqual( |
231 | ( |
232 | httplib.BAD_REQUEST, |
233 | - {'power_parameters': ["MAC Address: %s" % mac_error_msg]}, |
234 | + {'power_parameters': ["MAC Address: %s" % error_msg]}, |
235 | ), |
236 | (response.status_code, json.loads(response.content))) |
237 | |
238 | |
239 | === modified file 'src/maasserver/tests/test_fields.py' |
240 | --- src/maasserver/tests/test_fields.py 2014-02-21 03:49:40 +0000 |
241 | +++ src/maasserver/tests/test_fields.py 2014-04-23 07:31:18 +0000 |
242 | @@ -28,6 +28,8 @@ |
243 | NodeGroupFormField, |
244 | register_mac_type, |
245 | validate_mac, |
246 | + VerboseRegexField, |
247 | + VerboseRegexValidator, |
248 | ) |
249 | from maasserver.models import ( |
250 | MACAddress, |
251 | @@ -222,6 +224,35 @@ |
252 | pass |
253 | |
254 | |
255 | +class TestVerboseRegexValidator(MAASServerTestCase): |
256 | + |
257 | + def test_VerboseRegexValidator_validates_value(self): |
258 | + validator = VerboseRegexValidator( |
259 | + regex="test", message="Unknown value") |
260 | + self.assertIsNone(validator('test')) |
261 | + |
262 | + def test_VerboseRegexValidator_validation_error_includes_value(self): |
263 | + message = "Unknown value: %(value)s" |
264 | + validator = VerboseRegexValidator(regex="test", message=message) |
265 | + value = factory.make_name('value') |
266 | + error = self.assertRaises(ValidationError, validator, value) |
267 | + self.assertEqual(message % {'value': value}, error.message) |
268 | + |
269 | + |
270 | +class TestVerboseRegexField(MAASServerTestCase): |
271 | + |
272 | + def test_VerboseRegexField_accepts_valid_value(self): |
273 | + field = VerboseRegexField(regex="test", message="Unknown value") |
274 | + self.assertEqual('test', field.clean('test')) |
275 | + |
276 | + def test_VerboseRegexField_validation_error_includes_value(self): |
277 | + message = "Unknown value: %(value)s" |
278 | + field = VerboseRegexField(regex="test", message=message) |
279 | + value = factory.make_name('value') |
280 | + error = self.assertRaises(ValidationError, field.clean, value) |
281 | + self.assertEqual([message % {'value': value}], error.messages) |
282 | + |
283 | + |
284 | class TestMACAddressField(MAASServerTestCase): |
285 | |
286 | def test_mac_address_is_stored_normalized_and_loaded(self): |
287 | |
288 | === modified file 'src/maasserver/tests/test_forms.py' |
289 | --- src/maasserver/tests/test_forms.py 2014-04-10 15:30:11 +0000 |
290 | +++ src/maasserver/tests/test_forms.py 2014-04-23 07:31:18 +0000 |
291 | @@ -47,6 +47,8 @@ |
292 | INTERFACES_VALIDATION_ERROR_MESSAGE, |
293 | list_all_usable_architectures, |
294 | MACAddressForm, |
295 | + MAX_MESSAGES, |
296 | + merge_error_messages, |
297 | NetworkForm, |
298 | NewUserCreationForm, |
299 | NO_ARCHITECTURES_AVAILABLE, |
300 | @@ -274,7 +276,7 @@ |
301 | self.assertFalse(form.is_valid()) |
302 | self.assertEqual(['mac_addresses'], list(form.errors)) |
303 | self.assertEqual( |
304 | - ['Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF).'], |
305 | + ["'invalid' is not a valid MAC address."], |
306 | form.errors['mac_addresses']) |
307 | |
308 | def test_NodeWithMACAddressesForm_multiple_invalid(self): |
309 | @@ -287,7 +289,11 @@ |
310 | self.assertFalse(form.is_valid()) |
311 | self.assertEqual(['mac_addresses'], list(form.errors)) |
312 | self.assertEqual( |
313 | - ['One or more MAC addresses is invalid.'], |
314 | + [ |
315 | + "One or more MAC addresses is invalid. " |
316 | + "('invalid_1' is not a valid MAC address. \u2014" |
317 | + " 'invalid_2' is not a valid MAC address.)" |
318 | + ], |
319 | form.errors['mac_addresses']) |
320 | |
321 | def test_NodeWithMACAddressesForm_empty(self): |
322 | @@ -782,6 +788,38 @@ |
323 | list(form.fields)) |
324 | |
325 | |
326 | +class TestMergeErrorMessages(MAASServerTestCase): |
327 | + |
328 | + def test_merge_error_messages_returns_summary_message(self): |
329 | + summary = factory.make_name('summary') |
330 | + errors = [factory.make_name('error') for _ in range(2)] |
331 | + result = merge_error_messages(summary, errors, 5) |
332 | + self.assertEqual( |
333 | + "%s (%s)" % (summary, ' \u2014 '.join(errors)), result) |
334 | + |
335 | + def test_merge_error_messages_includes_limited_number_of_msgs(self): |
336 | + summary = factory.make_name('summary') |
337 | + errors = [ |
338 | + factory.make_name('error') |
339 | + for _ in range(MAX_MESSAGES + 2)] |
340 | + result = merge_error_messages(summary, errors) |
341 | + self.assertEqual( |
342 | + "%s (%s and 2 more errors)" % ( |
343 | + summary, ' \u2014 '.join(errors[:MAX_MESSAGES])), |
344 | + result) |
345 | + |
346 | + def test_merge_error_messages_with_one_more_error(self): |
347 | + summary = factory.make_name('summary') |
348 | + errors = [ |
349 | + factory.make_name('error') |
350 | + for _ in range(MAX_MESSAGES + 1)] |
351 | + result = merge_error_messages(summary, errors) |
352 | + self.assertEqual( |
353 | + "%s (%s and 1 more error)" % ( |
354 | + summary, ' \u2014 '.join(errors[:MAX_MESSAGES])), |
355 | + result) |
356 | + |
357 | + |
358 | class TestMACAddressForm(MAASServerTestCase): |
359 | |
360 | def test_MACAddressForm_creates_mac_address(self): |
Nice improvement, thanks! Notes follow:
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”?
.
What is VerboseRegexVal idator. code for? It looks mysterious. Comment?
.
The docstring for VerboseRegexVal idator. __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.
.
VerboseRegexFie ld.__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!
.
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?
.
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 cl...