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.

Sign up for GitHub

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

Jump to bottom

Fix parsing corner case closes #19382 #19529

Merged
merged 3 commits into from
Feb 6, 2018
Merged

Fix parsing corner case closes #19382 #19529

merged 3 commits into from
Feb 6, 2018

Conversation

jbrockmendel
Copy link
Member

@codecov
Copy link

codecov bot commented Feb 4, 2018 •
edited
Loading

Codecov Report

Merging #19529 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #19529   +/-   ##
=======================================
  Coverage   91.62%   91.62%           
=======================================
  Files         150      150           
  Lines       48714    48714           
=======================================
  Hits        44633    44633           
  Misses       4081     4081
Flag Coverage Δ
#multiple 89.99% <ø> (ø) ⬆️
#single 41.75% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc1d027...0af64f6. Read the comment docs.

jreback
jreback requested changes Feb 4, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

pls always always add a whatsnew note for any user facing change.

The conversion code for datetimes is still all over the place because tslib.pyx still exists, would be happy to remove that.

pandas/_libs/tslib.pyx Outdated
@@ -725,6 +731,21 @@ cpdef array_to_datetime(ndarray[object] values, errors='raise',
return oresult


cdef bint _handle_error_require_iso8601(object val, int64_t* iresult,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather do this inline, you are mutating the result here, and it makes the logic much harder to follow. If you want to do this in a separate PR that just refactors might be ok, but not on this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

pandas/tests/scalar/test_timestamp.py Outdated
@@ -15,6 +15,7 @@

from pandas.tseries import offsets

from pandas._libs.tslib import OutOfBoundsDatetime
Copy link
Contributor

Choose a reason for hiding this comment

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

from pandas.errors import

pandas/tests/indexes/datetimes/test_tools.py
@@ -1596,6 +1595,19 @@ def test_coerce_of_invalid_datetimes(self):
)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

import from pandas.errors

@jreback jreback added Bug Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas and removed Bug labels Feb 4, 2018
@jbrockmendel
Copy link
Member Author

The conversion code for datetimes is still all over the place because tslib.pyx still exists, would be happy to remove that.

Yah, I've been holding off on that because there are small differences between array_to_datetime and the Timestamp constructor that I'm hoping to un-difference, then share the implementation.

@jreback jreback added this to the 0.23.0 milestone Feb 6, 2018
@jreback jreback merged commit 84522a0 into pandas-dev:master Feb 6, 2018
@jreback
Copy link
Contributor

jreback commented Feb 6, 2018

thanks. happy to have a cleanup in array_to_datetime, but that's a separate PR

@jbrockmendel jbrockmendel deleted the corner_parse branch February 11, 2018 21:39
harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
@jbrockmendel
Fix parsing corner case closes pandas-dev#19382 (pandas-dev#19529)
9985077
@varadgunjal varadgunjal mentioned this pull request Nov 28, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

BUG: Out of bounds Timestamp does not raise exception
2 participants
@jbrockmendel @jreback