Skip to content
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

DOC: Fix docstring timestamps (Issue #59458) #59688

Merged

Conversation

mimistiv
Copy link
Contributor

@mimistiv mimistiv commented Sep 2, 2024

@MarcoGorelli
Copy link
Member

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

@mimistiv
Copy link
Contributor Author

mimistiv commented Sep 2, 2024

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
Copy link
Contributor Author

mimistiv commented Sep 3, 2024

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.

@MarcoGorelli
Copy link
Member

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

@mimistiv
Copy link
Contributor Author

mimistiv commented Sep 5, 2024

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
Copy link
Contributor Author

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!

@MarcoGorelli
Copy link
Member

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

Copy link
Contributor

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 github-actions bot added the Stale label Oct 17, 2024
@MarcoGorelli
Copy link
Member

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

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks @mimistiv

sorry for the delay, I wanted to check why it was necessary to use _year instead of using super().year - as far as I can tell, the answer is that pandas supports negative dates (i.e. before year 0) whereas the stdlib timestamp doesn't

@mimistiv
Copy link
Contributor Author

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?

@MarcoGorelli
Copy link
Member

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 MarcoGorelli merged commit 156e67e into pandas-dev:main Nov 11, 2024
50 of 51 checks passed
@MarcoGorelli MarcoGorelli changed the title Fix docstring timestamps (Issue #59458) DOCS: Fix docstring timestamps (Issue #59458) Nov 11, 2024
@MarcoGorelli MarcoGorelli added this to the 3.0 milestone Nov 11, 2024
@MarcoGorelli MarcoGorelli changed the title DOCS: Fix docstring timestamps (Issue #59458) DOC: Fix docstring timestamps (Issue #59458) Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants