Conversation
larsevj
force-pushed
the
use_ruff_2
branch
4 times, most recently
from
cf3d5fc to
eff4e60
Compare
larsevj
force-pushed
the
use_ruff_2
branch
4 times, most recently
from
8672735 to
825bbb9
Compare
eivindjahren
left a comment
eivindjahren
left a comment
There was a problem hiding this comment.
The test coverage of resdata is not great, and from earlier experiences, sweaping changes like this has a high risk of introducing bugs. I think we should increase the test coverage for the code being changed and ensure we don't change any behavior. I marked up a couple of cases where I know the test data is difficult to generate and I will have a look at helping out on those.
| if monitor_survey is not None: | ||
| if not monitor_survey in self: | ||
| raise KeyError("No such survey: %s" % monitor_survey) | ||
| if monitor_survey is not None and not monitor_survey in self: |
There was a problem hiding this comment.
The tests never check that these KeyErrors is raised under correct conditions. I think before merging this PR, we should add some tests that check that we don't accidentally change the condition for which the KeyError is raised.
| if I1 < 0 or self.nx <= I1: | ||
| raise ValueError("Invalid I1:%d" % I1) | ||
| if I2 < 0 or I2 >= self.nx: | ||
| if I2 < 0 or self.nx <= I2: |
There was a problem hiding this comment.
Except for the ValueError on line 227 and on line 232, these ValueErrors are not checked in the tests. I think before merging this PR, we should add some tests that check that we don't accidentally change the condition for which the ValueError is raised.
| polyline = target.getPolyline(k) | ||
| else: | ||
| polyline = target | ||
| polyline = target.getPolyline(k) if isinstance(target, Fault) else target |
There was a problem hiding this comment.
In the tests, target is always a Fault. We should add a test for which target is a polyline.
There was a problem hiding this comment.
Should probably be a check in resdata if input is neither a Fault nor polyline.
There was a problem hiding this comment.
In that case I think you will get a TypeError later on. Could just try it and then create a better error message.
| name = "Extend:%s" % self.getName() | ||
| else: | ||
| name = None | ||
| name = "Extend:%s" % self.getName() if self.getName() else None |
There was a problem hiding this comment.
extend_to_b_box is never called in the tests. We should add some tests for it before changing this function.
There was a problem hiding this comment.
I see that there is a similar extendToBBox for cpolylines, so could probably have removed most of this function, but can write some tests for it anyway.
| other_polyline = other.getPolyline(k) | ||
| else: | ||
| other_polyline = other | ||
| other_polyline = other.getPolyline(k) if isinstance(other, Fault) else other |
There was a problem hiding this comment.
end_join is never called with other being a Fault. We should add a test for that so we don't accidentally change the function.
| default = CTime(default_value) | ||
| except: | ||
| except Exception as err: | ||
| raise ValueError( |
There was a problem hiding this comment.
This ValueError is never raised from the tests.
|
|
||
| def __div__(self, divisor): | ||
| if isinstance(divisor, int) or isinstance(divisor, float): | ||
| if isinstance(divisor, (int, float)): |
There was a problem hiding this comment.
This condition is never false in the tests.
| if type(self) == type(value): | ||
| # This is a copy operation | ||
| self._memcpy(value) | ||
| elif isinstance(value, (int, float)): |
There was a problem hiding this comment.
This condition is never false in the tests.
| name = "%s" % name | ||
| else: | ||
| name = "[no name]" | ||
| name = "%s" % name if name else "[no name]" |
There was a problem hiding this comment.
This repr is never called in the tests.
2 participants
I commented out the Isort rules, as they messed up and you run into errors with circular imports and partially imported modules.
Defer the fix of that to another issue or pr.