Fix to_csv delimiter handling of timestamp format #7023
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidwendt
added
bug
davidwendt
requested review from
karthikeyann,
nvdbaranec,
cwharris and
shwina
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #7023 +/- ##
===============================================
+ Coverage 82.03% 82.23% +0.19%
===============================================
Files 96 96
Lines 16381 16547 +166
===============================================
+ Hits 13438 13607 +169
+ Misses 2943 2940 -3
Continue to review full report at Codecov.
|
galipremsagar
approved these changes
Dec 16, 2020
karthikeyann
approved these changes
Dec 16, 2020
Yes, that already works with every type. |
|
rerun tests |
nvdbaranec
approved these changes
Dec 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #6699
The timestamp format(s) used by the CSV writer have the form
%Y-%m-%dT%H:%M:%SZ. This means if the column delimiter','or the line delimiter\nis either':'or'-'then the timestamp string output could conflict with these delimiters. The current logic simply removed these delimiters from the format if they detected a conflicting column or line delimiter. For example, specifying a dash'-'as column delimiter caused the timestamp format to change to%Y%m%d...(the dash is removed). I admit this was kind of hacky and also made the output inconsistent with Pandasto_csv().It is easy enough to simply add double-quotes around the timestamp format to prevent these conflicts as well as make the output consistent. This PR fixes that logic.
Exception logic to check for a dash as column separator was also found in csv.py, specifically citing issue 6699 in the exception message. Also, there was a pytest specifically created to check for this exception. The exception is removed and the pytest function updated in this PR as well.