-
Notifications
You must be signed in to change notification settings - Fork 6
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
Issue #145 #146
Issue #145 #146
Conversation
Check if destination is a date. If it is then should:
|
if isinstance(input_data, datetime.datetime): | ||
input_data = str(input_data).replace(' ', '_') | ||
warnings.warn( | ||
f"Found destination with a datetime format ({str(input_data)}). " \ |
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 wonder if at this point input_data
is not already a str
with white space replaced by underscore? This would make the warning a little confusing.
I would suggest to not reassign input_data
but rather create a new variable result
(or whatever name you prefer) to be able to report on the original data.
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.
yeap, nice catch, thanks:)
typing-extensions | ||
pyarrow |
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 wonder why we did not need pyarrow
in the requirements before?
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 just have changed the order to have the same as we have in the requirements_pandas1.txt
file.
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.
Thanks for the PR - looks good!
I have one comment regarding the warning message and another regarding the requirements for testing with pandas 2 that I would like to understand before approving to merge this PR.
Solving issue #145