pandas icon indicating copy to clipboard operation
pandas copied to clipboard

Fix docstring timestamps (Issue #59458)

Open mimistiv opened this issue 1 year ago • 9 comments

  • [x] Addresses some tasks of #59458
  • [ ] Tests added and passed if fixing a bug or adding a new feature
  • [x] All code checks passed.
  • [x] Added type annotations to new arguments/methods/functions.
  • [ ] Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

mimistiv avatar Sep 02 '24 17:09 mimistiv

thanks @mimistiv ! 🙏

i'll take a closer look later, but I think that year might need defining in some other places - i'd suggest checking how/where month is defined and taking inspiration from there

or just splitting year off from this PR and doing it separately, so we can merge month here quickly

MarcoGorelli avatar Sep 02 '24 18:09 MarcoGorelli

thanks @mimistiv ! 🙏

i'll take a closer look later, but I think that year might need defining in some other places - i'd suggest checking how/where month is defined and taking inspiration from there

or just splitting year off from this PR and doing it separately, so we can merge month here quickly

thanks @MarcoGorelli ! I think the issue is related to https://github.com/pandas-dev/pandas/blob/main/pandas/_libs/tslibs/timestamps.pxd#L24, where nanosecond and year are already declared

mimistiv avatar Sep 02 '24 20:09 mimistiv

hey @MarcoGorelli! Do you have any advice on how I could best handle this issue? I was thinking I could change the declarations in the pxd file to use _year and _nanosecond similarly to _value but that will mean I need to change it everywhere and I need to be sure what all the uses are. In the meantime I might create a separate PR to handle the value changes and the month small fix.

mimistiv avatar Sep 03 '24 15:09 mimistiv

hey - sure, let's start with a separate pr which we can merge quickly, and then we'll iterate over the trickier one?

MarcoGorelli avatar Sep 03 '24 17:09 MarcoGorelli

hey - sure, let's start with a separate pr which we can merge quickly, and then we'll iterate over the trickier one?

Hey @MarcoGorelli , I split up the PR and the value/month part of it has now been merged! :) Could you please take a look at the suggested solution I have for year and nanosecond here?

mimistiv avatar Sep 05 '24 16:09 mimistiv

Hey @MarcoGorelli @mroeschke, could I ask one of you to please take a look at the proposed solution in this PR when you have the time? Thanks in advance!

mimistiv avatar Sep 16 '24 14:09 mimistiv

hey - sure, sorry for the delay, i still need to undertsand why year needed to be marked as cdef readonly in the first place, it's already a property of the python standard library's datetime.datetime...will get back to you on this

MarcoGorelli avatar Sep 16 '24 14:09 MarcoGorelli

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

github-actions[bot] avatar Oct 17 '24 00:10 github-actions[bot]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

my bad unfortunately, just had a work trip + holiday, and very little / no funded time at the moment - i'll get back to this, i'll remove the 'stale' label

MarcoGorelli avatar Oct 17 '24 09:10 MarcoGorelli

Thanks @MarcoGorelli ! I can see that merging with main resulted in a failed unit test which seems unrelated to my PR. Is this a known issue?

mimistiv avatar Nov 11 '24 16:11 mimistiv

agree that it's unrelated, but recent commits look green 🥦 so it may have just fixed itself - let's ship this then, thanks for your contribution!

MarcoGorelli avatar Nov 11 '24 16:11 MarcoGorelli