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

ENH: make guess_datetime_format public #55079

Merged
merged 35 commits into from
Oct 27, 2023

Conversation

nnlnr
Copy link
Contributor

@nnlnr nnlnr commented Sep 9, 2023

@nnlnr nnlnr marked this pull request as ready for review September 9, 2023 17:56
@nnlnr nnlnr requested a review from MarcoGorelli as a code owner September 9, 2023 17:56
@@ -42,7 +42,7 @@ Bug fixes

Other
~~~~~
-
- Enhancement in :meth:`_libs.tslibs.parsing.guess_datetime_format` to make public (:issue:`54727`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Enhancement in :meth:`_libs.tslibs.parsing.guess_datetime_format` to make public (:issue:`54727`)
- :meth:`pandas.tseries.guess_datetime_format` is now available (:issue:`54727`)

Can you move this to the other enhancement section?

Copy link
Member

Choose a reason for hiding this comment

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

Also this should be in 2.2.0.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIll do - thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved enhancement note to 2.2.0.rst unde the Other Enhancements section ✅

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Could you add this to the API reference documentation?

@mroeschke mroeschke added the Datetime Datetime data dtype label Sep 11, 2023
@nnlnr
Copy link
Contributor Author

nnlnr commented Sep 13, 2023

Could you add this to the API reference documentation?

is the API reference documentation in doc/source/reference/api? If so, I'm seeing that path in the .gitignore file - is there a procedure to update the API reference under this context?

@mroeschke
Copy link
Member

I think you can just add an entry to doc/source/reference/general_functions.rst

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.

thanks for your pr

whilst we're changing this

  • in the guess_Datetime_format docstring, where it says
    Warning: dayfirst=True is not strict, but will prefer to parse
    with day first (this is a known bug).

could you write that as a sphinx directive? so .. warning::. check the to_datetime docs, there's something similar there

  • could we add an example to the docstring please? just a simple one-liner of it guessing a datetime format, and another one-liner of it not being able to guess another one, will do

@nnlnr
Copy link
Contributor Author

nnlnr commented Sep 14, 2023

Thanks for the review and comments @mroeschke and @MarcoGorelli

  • added an entry to doc/source/reference/general_functions.rst ✅
  • updated warning in docstring as sphinx directive ✅
  • added examples to docstring ✅

@MarcoGorelli
Copy link
Member

Can you fix the failing ci checks please

@nnlnr
Copy link
Contributor Author

nnlnr commented Sep 16, 2023

👋 I'm getting a bit stuck with these failing checks - which seem to be mostly due to the following attribute error:
AttributeError: module 'pandas' has no attribute 'guess_datetime_format'

I updated the docstring in parsing.pyx with a sphinx ..warning:: directive and added two examples (learned that formatting was important which was a good lesson); however not sure how to address the failing jobs - any feedback or advice on how to troubleshoot?

@github-actions
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 20, 2023
@MarcoGorelli
Copy link
Member

hi @nnlnr - is this still active? if you could fixup the conflicts I can finish it up

@nnlnr
Copy link
Contributor Author

nnlnr commented Oct 22, 2023

👋 @MarcoGorelli - thanks! That would be great - let me fix up the conflicts and circle back here

@MarcoGorelli MarcoGorelli added this to the 2.2 milestone Oct 26, 2023
@MarcoGorelli
Copy link
Member

website build looks good:

image

merging then, thanks @nnlnr !

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.

going to need @mroeschke to approve before this can be merged

Comment on lines 62 to 66
Top-level dealing with Interval data
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. autosummary::
:toctree: api/
Copy link
Member

Choose a reason for hiding this comment

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

extra line, but it doesn't seem to make any difference in the build so is OK imo

image

@mroeschke
Copy link
Member

Could you also add a whatsnew note in 2.2 under the Other Enhancements section?

@mroeschke mroeschke merged commit beed6bc into pandas-dev:main Oct 27, 2023
29 of 33 checks passed
@mroeschke
Copy link
Member

Thanks @nnlnr and @MarcoGorelli

@nnlnr
Copy link
Contributor Author

nnlnr commented Oct 30, 2023

Thanks @MarcoGorelli @mroeschke!! 🎉 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: make guess_datetime_format public
3 participants