Fix chained offset bug #5031
Fix chained offset bug #5031
Conversation
MetRonnie
force-pushed
the
conjugate-duration-bug
branch
from
b97d367
to
8d22e33
Compare
| context_start_point, _ = self._get_point_from_expression( | ||
| context_start_point, None) |
There was a problem hiding this comment.
FYI: This is subjective, the way this was done before saved an unnecessary assignment.
| ptslist.append(cpoint) | ||
| if cpoint == min(ptslist): | ||
| min_entry = point |
There was a problem hiding this comment.
>>> min([1, None])
Traceback ...
TypeErrorHow has this one not blown up before?
Hole in the tests?
There was a problem hiding this comment.
Not sure. Perhaps it is very unlikely to be hit. It's difficult to tell because the self._get_point_from_expression() method probably does too much for a single method (no unit tests to tell us what it's supposed to be doing).
As for Mypy not flagging the appending None to List[TimePoint], I think that might be due to the fact we're missing py.typed in isodatetime (added in metomi/isodatetime#203)
|
oops |
MetRonnie
force-pushed
the
conjugate-duration-bug
branch
from
c52f5aa
to
b80aa50
Compare
|
(Fast test failure on MacOS due to Codecov uploader failing again...) |
| @@ -355,78 +359,69 @@ def _get_interval_from_expression( | |||
| def _get_min_from_expression( | |||
| self, | |||
| expr: str, | |||
| context: 'TimePoint' | |||
| context: Optional['TimePoint'] | |||
| ) -> str: | |||
There was a problem hiding this comment.
(Sighing) Yes it would have. When someone figures out what this is doing it would also be a good idea to add a unit test
There was a problem hiding this comment.
Out of scope to be fair.
I wrote a longer comment that I also deemed OOS about how many of these might be better as static methods fed values as args to enable nice unit testing.
| allow implicit tasks = True | ||
| [scheduling] | ||
| initial cycle point = {icp} | ||
| final cycle point = {fcp_expr} |
There was a problem hiding this comment.
I'm not sure if _parse_chain_expression is used elsewhere, but if it is, it's worth noting that this only works if the sum of the durations > 0.
Also, should
('2000-01-01T00:00', '+PT6H-PT1S', '2000-01-01T05:59'),Work?
Might be worth some additional tests here for intervals not P1D and P1M?
There was a problem hiding this comment.
- I've added more tests
- I've opened an issue for the
+PT6H+PT1Sbug: Chain offset expression with time units does not work #5047 parse_chain_expression()works for net < 0 durations but that does not produce a valid final cycle point, because it is before the initial cycle point (already a tested scenario)
Fix conjugate duration bug * Closes cylc#4908 * Fix bug where specifying multiple datetime offsets would not obey the given order.
These changes close #4908
Fix bug where specifying multiple datetime offsets would not obey the given order.
E.g.
final cycle point = +P1M-P1Dwould lead to it calculatingicp - P1D + P1M(smallest unit to largest unit order) instead oficp + P1M - P1D(given order).Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfgandconda-environment.yml.