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

Time overflow #2232

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@d-chambers
Member

d-chambers commented Oct 8, 2018

What does this PR do?

Allows for hour, minute, and second values to exceed the normal limits of 23, 59, 59 when creating a UTCDateTime object from key word arguments or using the replace method.

Why was it initiated? Any relevant Issues?

This change was discussed in #2222. It will allow several format parsers to favor using this approach when dealing with time values that may exceed their normal limits, rather than adding ad-hoc checks/handling logic in each parser.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 8, 2018

Member

+DOCS

Member

d-chambers commented Oct 8, 2018

+DOCS

Show resolved Hide resolved CHANGELOG.txt Outdated
Show outdated Hide outdated obspy/core/utcdatetime.py Outdated
@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 9, 2018

Member

@QuLogic thanks for the review!

Member

d-chambers commented Oct 9, 2018

@QuLogic thanks for the review!

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 9, 2018

Member

Allows for hour, minute, and second values to exceed the normal limits of 23, 59, 59 when creating a UTCDateTime object from key word arguments or using the replace method.

Hmm, @d-chambers I think there might have been a bit of an misunderstanding in #2222. What I wanted to propose there is to leave UTCDateTime.__init__() as is (i.e. keep raising exceptions on overflow in hour, minute etc., which is in line with builtin datetime behavior) or at most change it so it allows one special case for exactly 24:00:00.0 (which already deviates from Python builtin datetime but is explicitly allowed by ISO 8601). I wanted to suggest to only add an internal helper routine to generate UTCDateTimes from overflowing kwargs in specific situations in I/O submodules.

Personally, I think we should not promote abusing UTCDateTime with input that is not ISO conform. It might obscure bugs in user land codes in my opinion.

But I'm ready to put this to a vote of course.

Member

megies commented Oct 9, 2018

Allows for hour, minute, and second values to exceed the normal limits of 23, 59, 59 when creating a UTCDateTime object from key word arguments or using the replace method.

Hmm, @d-chambers I think there might have been a bit of an misunderstanding in #2222. What I wanted to propose there is to leave UTCDateTime.__init__() as is (i.e. keep raising exceptions on overflow in hour, minute etc., which is in line with builtin datetime behavior) or at most change it so it allows one special case for exactly 24:00:00.0 (which already deviates from Python builtin datetime but is explicitly allowed by ISO 8601). I wanted to suggest to only add an internal helper routine to generate UTCDateTimes from overflowing kwargs in specific situations in I/O submodules.

Personally, I think we should not promote abusing UTCDateTime with input that is not ISO conform. It might obscure bugs in user land codes in my opinion.

But I'm ready to put this to a vote of course.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 9, 2018

Member

@megies not a problem. I can move this logic somewhere else. I would favor a public alternate constructor method on UTCDateTime, however, over an internal helper function as I have needed this sort of logic in parsing several files, not just within obspy. I won't have time to do this for a few days.

Member

d-chambers commented Oct 9, 2018

@megies not a problem. I can move this logic somewhere else. I would favor a public alternate constructor method on UTCDateTime, however, over an internal helper function as I have needed this sort of logic in parsing several files, not just within obspy. I won't have time to do this for a few days.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 9, 2018

Member

.. or another option might be to leave it in UTCDateTime.__init__() like you have right now but only switch it on explicitly on request with something like UTCDateTime.__init__(..., strict=True) (or loose=False, or sloppy=False, or ...)

I won't have time to do this for a few days.

sure no worries

Member

megies commented Oct 9, 2018

.. or another option might be to leave it in UTCDateTime.__init__() like you have right now but only switch it on explicitly on request with something like UTCDateTime.__init__(..., strict=True) (or loose=False, or sloppy=False, or ...)

I won't have time to do this for a few days.

sure no worries

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 9, 2018

Member

Ya, I like the idea of a 'strict' keyword. I may still move most the code to another method though to cleanup the init (it is getting quite long)

Member

d-chambers commented Oct 9, 2018

Ya, I like the idea of a 'strict' keyword. I may still move most the code to another method though to cleanup the init (it is getting quite long)

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 11, 2018

Member

Ok @megies, take a look at see if this is more to your liking. I am still trying to leave the overflow behavior in a try/except clause in order to not slow down the __init__ when the overflow logic is not needed.

Member

d-chambers commented Oct 11, 2018

Ok @megies, take a look at see if this is more to your liking. I am still trying to leave the overflow behavior in a try/except clause in order to not slow down the __init__ when the overflow logic is not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment