num2date: return datetimes by default if possible #165
Conversation
|
There is Test Carnage, but I'm not (yet) convinced it's because I've done something wrong... |
|
Right. The reason my original effort didn't work was because we call To do this properly, we should probably sort out #166, but I think that's beyond the scope of this PR. |
|
Why are tests not running for my latest commit? 😕 |
|
All tests now passing, except for some doctests (and a whole raft of python2.7 tests) which also fail at #164, so not caused by this change. |
|
Rebased. Tests now all passing 🙂 |
|
Thanks for looking into this @rcomer 💐 Having discussed with a few devs, I don't want to merge this to To avoid future difficulties/irrelevance a package should evolve with its dependencies, so as @duncanwp hinted at it in #161: the next major release of cf-units should explicitly adopt the new default behaviour of Pinning the behaviour to how it was in the 'old world' is a bugfix, enabling continued use of the existing major release of cf-units. As I understand it, bugfix changes can/should only be applied to a bugfix branch, and that certainly seems appropriate here given the conflict with an opposite intended future behaviour. I'm just making sure I fully understand how this ought to be done. I had started working on a migration to Cirrus+Nox, but at least for now Travis seems to be playing nicely so I'll only involve that if I need to! |
|
Thanks @trexfeathers, I shall await further instructions. |
There was a problem hiding this comment.
In terms of the code, this looks good. One change if you can:
Also need an equivalent change when using cftime.utime - L1937, and propagate upwards for the container method and uses.
You're definitely right to move away from using cftime.utime where possible, since it's on its way out. But as of v1.4.1 it's still in the cftime API and it also has a only_use_cftime_datetimes parameter - see here.
|
@trexfeathers have I understood correctly? If the Unfortunately, having made and reverted changes to |
Oh dear, I was only suggesting an additional change, not undoing your existing! While you are correct, I was trying to say that your changes up to that point had been valuable anyway:
|
This reverts commit 0b89777.
|
Okedoke. Commit histories are fun! I believe this shows the changes since your initial review. Does the |
|
Also, do you have a "squash" option in your merge button or should I do that manually? |
There was a problem hiding this comment.
Looks good @rcomer. If you can target the new v2.1.5 branch I'm good to merge.
Does the
utimemethod need a test? I can't find any existing ones for that method...
Since you didn't introduce this method, and given its future limited life in cftime, I'm happy to forego additional tests.
Also, do you have a "squash" option in your merge button or should I do that manually?
I do have the squash option - I just need to remember to use it!
|
Thanks @trexfeathers, now re-targetted. |
* make proper datetimes default * replace cftime.utime.num2date with cftime.num2date * attempt fixes * pass both utime and Unit to _num2date_to_nearest_second * revert _num2date_to_nearest_second calling sequence * dummy commit to kick travis * appease stickler * license headers * contain only_use_cftime_datetimes within utime object * revert test__num2date_to_nearest_second.py * actually pass flag to where it is needed * reinstate type check * Reinstate _num2date_to_nearest_second changes This reverts commit 0b89777. * reinstate test changes * make sure option is passed down num2date
* num2date: return datetimes by default if possible (#165) * make proper datetimes default * replace cftime.utime.num2date with cftime.num2date * attempt fixes * pass both utime and Unit to _num2date_to_nearest_second * revert _num2date_to_nearest_second calling sequence * dummy commit to kick travis * appease stickler * license headers * contain only_use_cftime_datetimes within utime object * revert test__num2date_to_nearest_second.py * actually pass flag to where it is needed * reinstate type check * Reinstate _num2date_to_nearest_second changes This reverts commit 0b89777. * reinstate test changes * make sure option is passed down num2date * change num2date default to cftime.datetimes * change num2date tests expected type * remove utime * replace cftime julian date functions * pep8 * update matplotlib comments * pep8 * specify flag in matplotlib note * review actions * update utilities.rst
Aims to fix #161. I don't know how to run the tests (or anything else) locally with my branch, so I am trusting to Travis! I did run
flake8locally on the files I’ve changed, and it only complained about two lines that I haven’t touched.