Conversation
Checks for valid_range. If it is not present, this is followed by valid_min and valid_max check. Implements ioos#38.
| self.base_check._check_min_max_range(var, tc2) | ||
| expected = ['{} attribute should exist, have the same type as pressure, and not be empty or valid_range should be defined'.format(v) | ||
| for v in ['valid_min', 'valid_max']] | ||
| self.assertEqual(expected, tc2.messages) |
There was a problem hiding this comment.
Looking good. Can you add a docstring for these tests about the specific cases that are being tested?
| # otherwise compare the numpy types directly | ||
| else: | ||
| test_ctx.assert_true(v_bound.dtype == var.dtype, warn_msg) | ||
| return test_ctx |
There was a problem hiding this comment.
Just for readability and maintainability I think it would be better to see the two branches broken into individual methods.
if 'valid_range' in var.ncattrs():
return self._check_valid_range_attr(var, test_ctx)
else:
return self._check_valid_minmax(var, test_ctx)There was a problem hiding this comment.
The reason I did not do this is because it'd need that same boilerplate every time we wanted to check for {valid_range|valid_min,valid_max}.
There was a problem hiding this comment.
Since it appears 5 times in the ncei_base.py module, I prefer the function form.
| test_ctx.assert_true(getattr(ncvar, 'valid_min', '') != '', 'valid_min should exist and not be empty') | ||
| test_ctx.assert_true(getattr(ncvar, 'valid_max', '') != '', 'valid_max should exist and not be empty') | ||
| test_ctx.assert_true(getattr(ncvar, 'valid_min', '') != '', 'valid_min should exist and not be empty or valid_range should be defined') | ||
| test_ctx.assert_true(getattr(ncvar, 'valid_max', '') != '', 'valid_max should exist and not be empty or valid_range should be defined') |
There was a problem hiding this comment.
Is this intentionally only run when ncvar does not have standard_name?
There was a problem hiding this comment.
Oops! I didn't realize there was this indent. I will fix that.
There was a problem hiding this comment.
This should actually be calling the function I defined instead.
|
Conda stahp 🛑 |
|
@benjwadams @ocefpaf |
|
Safe to remove b/c that is package deps error and not a |
| @@ -18,17 +18,16 @@ matrix: | |||
| env: TEST_TARGET=coding_standards | |||
|
|
|||
There was a problem hiding this comment.
Was necessary to add this to keep LXML from breaking due to a missing library. Might be worth looking at the latest LXML package in conda to see if it's an issue.
There was a problem hiding this comment.
This is certainly a bad dependency package on the conda side and we should not do that here.
I guess that is happening only with Python 3.4, which we dropped in #41, but if you do observe this in other Pythons then we should report it back to conda-forge.
No description provided.