Fix misleading error message when it is not possible to create area weights requested from cf.Field.collapse

[davidhassell]

Fixes #731

[sadielbartholomew]

Hi David, did you want me to review this? Asking since it seems you might be waiting on a review but I am not assigned.

[sadielbartholomew]

The fix seems to work and is logically sensible though I'd be happier to merge if we can, to future-proof this, have a test to check we get the right error message emerging for this case. I imagine it wouldn't be too much of a pain to set up for the case and then checking the ValueError message is trivial, but let me know if not or if for some other reason there is no test. (Bonus points if we can add further testing on error messages for other cases too, whilst at it.)

[sadielbartholomew]

+1 for this, since as discussed last week it would be best to decide the version numbered name at release-time.

[davidhassell]

Hi Sadie - got any ideas on how to test on the value of the exception message string? Is that possible?

[sadielbartholomew]

Hi @davidhassell, the way I am familiar with, and seems quite readable, is to update assertRaises to assertRaisesRegex: see https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaisesRegex. As an example using the test_Field module, this diff works to also test the error message:

diff --git a/cf/test/test_Field.py b/cf/test/test_Field.py
index 6f528642f..255ed45bd 100644
--- a/cf/test/test_Field.py
+++ b/cf/test/test_Field.py
@@ -393,7 +393,9 @@ class FieldTest(unittest.TestCase):
                         data=d,
                     )
 
-        with self.assertRaises(Exception):
+        with self.assertRaisesRegex(
+            ValueError, "Can't set components=True and data=True"
+        ):
             f.weights(components=True, data=True)
 
     def test_Field_replace_construct(self):

It would be good to (eventually) go in and update more/most/all error checking to also test the messages in that way: shall I open an Issue for it?

[davidhassell]

+1 - I'll try that

[davidhassell]

Done: 3c2802f

[sadielbartholomew]

Thanks for adding the test as requested, which passes but captures the bug at hand. I have sanity checked after the new commit. Lovely, please merge.