Skip to content
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 truncated timepoint addition bug #234

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

MetRonnie
Copy link
Contributor

@MetRonnie MetRonnie commented Jul 25, 2023

Closes #80
Closes #212

Built on #203

  • Tests included
  • Changelog entry included

@MetRonnie MetRonnie added the bug label Jul 25, 2023
@MetRonnie MetRonnie added this to the 3.1.0 milestone Jul 25, 2023
@MetRonnie MetRonnie self-assigned this Jul 25, 2023
@MetRonnie
Copy link
Contributor Author

Note this does not fix cylc/cylc-flow#2382, presumably that is still a bug in cylc-flow

@MetRonnie
Copy link
Contributor Author

Actually which should the behaviour be?

2019-01-15 + --01-15 = ?
  1. 2019-01-15 - same point (current behaviour)
  2. 2020-01-15 - next matching point (what I think it should be)

@MetRonnie MetRonnie marked this pull request as draft August 11, 2023 10:28
@MetRonnie

This comment was marked as outdated.

@MetRonnie MetRonnie modified the milestones: 3.1.0, 3.2.0 Oct 4, 2023
@MetRonnie
Copy link
Contributor Author

MetRonnie commented Oct 19, 2023

Actually which should the behaviour be?

2019-01-15 + --01-15 = ?
  1. 2019-01-15 - same point (current behaviour)
  2. 2020-01-15 - next matching point (what I think it should be)

I think we have to maintain the current behaviour to avoid breaking next() in Cylc workflows - see cylc/cylc-flow#5777

(Technically we could change the behaviour here and add a workaround in Cylc, but that's more effort)

@MetRonnie
Copy link
Contributor Author

MetRonnie commented Nov 2, 2023

Decision on 2019-01-15 + --01-15 (with greater clarity and precision):

The behaviour is to be:

2019-01-15T00:00:00 + --01-15
>>> 2019-01-15T00:00:00  # same datetime (we are exactly at midnight)

2019-01-15T00:00:01 + --01-15
>>> 2020-01-15T00:00:00  # next year 

This is because a truncated datetime should assume 0 for any unspecified time units, and 1 for any unspecified date units lower than what has been specified.

@MetRonnie MetRonnie force-pushed the truncated-timepoint-bug branch 3 times, most recently from fb0a2f0 to 2f75c13 Compare January 16, 2024 16:44
@MetRonnie MetRonnie marked this pull request as ready for review January 16, 2024 17:07
…in prior datetime

Truncated timepoint addition now correctly assumes unspecified units smaller than the largest specified unit are at their minimal values

`T00` is short for `T0000` and so `---01` should be short for `01T0000`, `--04` should be short for `--04-01T0000` etc.
Copy link
Contributor

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, code looks good and looks like it fixes the bug. Is it worth filling the gaps in the coverage whilst you're at it. I think it is.

Copy link
Contributor

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I failed to approve sooner. Please get a second reviewer or merge as you think appropriate.

@MetRonnie
Copy link
Contributor Author

No worries, this has not been a top priority at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants