Fix to csv2rec bug for review #6185
Conversation
Datetime strings in csv files in dayfirst or yearfirst format with nonzero hour&minute&second will not be properly represented. Date strings will, presuming the csv file contains no strings in that column that have nonzero hour/minute/second components in a datetime string.
lib/matplotlib/mlab.py
Outdated
| # try and return a datetime object | ||
| d = dateparser(x, dayfirst=dayfirst, yearfirst=yearfirst) | ||
| return d | ||
| mydateparser = with_default_value(mydateparser, datetime.datetime(1,1,1,0,0,0)) |
There was a problem hiding this comment.
This line has multiple pep8 violations (too long and lacking spaces after ,).
|
Can you add a test to |
|
This is also a candidate for backporting to 1.5.x |
|
Thanks @tacaswell! Ok, I missed pep8, I understand what the violations are and can fix them. I presume that I need to close the commit, then after modifying the code resubmit it - no drama. I've had a look at adding a test to test_mlab.py, but have no idea how to do it - for instance, the test object should be a .csv file, so it isn't as simple as calling csv2rec with a few arguments, and then checking that the result is as expected. Should I create a csv file with the test values in it within test_mlab.py (ie, manually create .csv file on the spot), then open this just-created file and check the values read out? I also don't understand the overall structure or mechanism of the test_mlab.py file or of unit tests for matplotlib, so is there any resource out there explaining how to add tests? I don't see any reference to existing csv2rec tests in there, so this looks like a new initiative, at least for csv2rec. I'll look at backporting once we've worked out all the above (!). Any guidance would be helpful - thanks. |
|
Ok, found the csv2rec tests in line 305 onwards of test_mlab.py. Now trying to understand them - they follow the model of creating temporary files, then testing that the results returned are as expected. |
|
You don't need to close the PR, just keep pushing new commits to your On Sun, Mar 20, 2016 at 3:20 AM, ultra-andy notifications@github.com
|
yearfirst & dayfirst date interpretation bug for datetime strings - csv2rec does not take yearfirst or dayfirst into account when interpreting datetime strings with a time component, so for ambiguous cases, the month and day get transposed. Test added also.
adding in datetime library import
removal of spaces around "="
|
Great, the auto tests now pass! Assuming that's all OK and gets approved, now how do I backport this fix to 1.5.x? I'm using the Windows GitHub interface, I've created a branch off 1.5.x, but have no idea where the 1.5.x files are or should be ?? Guidance appreciated... |
|
After this is merged we cherry-pick the merge commit back to the 1.5.x branch. You don't need to do anything extra. |
|
Excellent! A few questions about my workflow - is it OK to rely on the auto checks after submission to ensure adherence to PEP-8, or is this poor form? Also, any comments on code will be happily received - I don't have a lot of Python experience, so my code may have obvious improvements. |
|
It is best if you run the test locally before pushing (if nothing else, it helps to make your iteration loop much faster). I strongly suggest setting up a linter in what ever editor you use. PEP8 compliance can be annoying, but having a consistent style does help reading and understanding parts of the code you are not familiar with. Given the size of the mpl code base and the number of unique contributors we have we need to have some sort of consistent style and by picking PEP8 we fit in with everyone else and some one else writes the validation tools 😜 . |
|
The code in this PR is fine. It could have also been done via a lambda or |
…irst-bug FIX: csv2rec bug Closes #6184
…irst-bug FIX: csv2rec bug Closes #6184
|
backported to v1.5.x as a919ac7 |
|
Got it - I tried PyLint (integrated into PyScripter) and found it far stricter than the PEP-8 standard, then found autopep8, which runs at the command line, and that works very nicely. I'll try that next time prior to commit (if I ever find another bug annoying enough for me to want to fix it!) Just so I understand the background process a bit better, what makes you decide when something should go into 1.5.2, and when something should be held over for 2.0.1? And when do these get released? |
|
low-risk bug fixes get backported to the maintenance branch (current 1.5.x). The next feature release is planned to be 2.0 (with major style changes) and should be released as soon as we finish it. 1.5.2 will only be released if we need to, but we are accumulating bug-fixes on the 1.5.x branch just-in-case. |
…etime-dayfirst-bug FIX: csv2rec bug Closes matplotlib#6184
Datetime strings in csv files in dayfirst or yearfirst format with
nonzero hour&minute&second will not be properly represented. Date
strings will, presuming the csv file contains no strings in that column
that have nonzero hour/minute/second components in a datetime string. Refer to issue #6184 for further information.