-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
BUG: Fix parsing of ISO8601 durations #37159
Conversation
9cadda1
to
0a8fdad
Compare
("PT1S", Timedelta(seconds=1)), | ||
("PT0S", Timedelta(seconds=0)), | ||
("P1WT0S", Timedelta(days=7, seconds=0)), | ||
("P1D", Timedelta(days=1)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we handle negatives of these? (IIRC that's on one of the issues)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change the behavior of negativity, and the question is how to handle it. One of the only references I can find regarding the expected behavior is this comment, which also seems to indicate it's not very well defined in the ISO 8601 standard. How would you feel about pulling that out into a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes pls create a separate issue. only thing for now is do we raise if a negative is passed in? (i think we should unless / until we decide on how to handle it), though isn't the result just negative of the Timedelta itself which is well defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do.
No, nothing is raised when it is negative, and the behavior is, admittedly, quite odd, i.e.
"P-6DT0H50M3.010010012S"
parses as Timedelta( days=-6, minutes=50, seconds=3, milliseconds=10, microseconds=10, nanoseconds=12, )
, and the negative is only allowed right after the P descriptor. A negative in any other position will raise an error. As far as I can tell, there are two possibilities to go here:
- only support an overall "-" e.g.
"-P6DT1H" = Timedelta('-7 days +23:00:00')
and/or - support positive/negative on each, e.g.
"P7DT-1H3M" = Timedelta('6 days 23:03:00')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue now here: #37172. I would prefer to change/update the negativity behavior in a separate PR if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep that's great. well scoped PRs are good. one thing at a time. and thanks for working on this.
- ISO 8601 doesn't seem to express that explicit limitations here are necessary.
0a8fdad
to
3786f8b
Compare
thanks @mgmarino very nice! |
This PR fixes the following behavior:
Fixes #29773.
Fixes #36204.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff