-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
CLN: move maybe_casted_values from pandas/core/frame.py to pandas/core/dtype/cast.py #36985
CLN: move maybe_casted_values from pandas/core/frame.py to pandas/core/dtype/cast.py #36985
Conversation
For something thats only used once, im not sure it is worth moving. Definitely worth seeing if this can be simplified though, per a couple of TODO comments in there |
ee5e2e0
to
b59f39b
Compare
values_type = type(values) | ||
values_dtype = values.dtype | ||
|
||
from pandas.core.arrays.datetimelike import DatetimeLikeArrayMixin |
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.
not sure this is acceptable - will change if not. The issue is we can't just import at the top of the file because of a circular import
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.
you could do needs_i8_conversion(values.dtype) and isinstance(values, ExtensionArray)
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 fine with cleaning this up after, this routine definitely needs it (and also possibly is duplicative of things in this file anyways)
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.
you could do
needs_i8_conversion(values.dtype) and isinstance(values, ExtensionArray)
tried it and it broke some tests. I'll investigate in the follow-on
@jbrockmendel Will do! (in a followon) |
988354d
to
c349f82
Compare
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.
can you merge master and ping on green
values_type = type(values) | ||
values_dtype = values.dtype | ||
|
||
from pandas.core.arrays.datetimelike import DatetimeLikeArrayMixin |
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 fine with cleaning this up after, this routine definitely needs it (and also possibly is duplicative of things in this file anyways)
c349f82
to
20c2620
Compare
ping on green-ish (meaning either green or a specific buidl is failing on completely unrelated; though I think @jbrockmendel may have fixed this). |
thanks @arw2019 if you can rebase the typing one on master would be great. |
Thanks @jreback @jbrockmendel for reviewing!
Done |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
xref #36876, #27370 (stale PR)