-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix misleading error message when it is not possible to create area weights requested from cf.Field.collapse
#732
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
@@ -1,3 +1,14 @@ | |||
version NEXT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for this, since as discussed last week it would be best to decide the version numbered name at release-time.
Hi Sadie - got any ideas on how to test on the value of the exception message string? Is that possible? |
Hi @davidhassell, the way I am familiar with, and seems quite readable, is to update 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? |
+1 - I'll try that |
Done: 3c2802f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fixes #731