Skip to content

Improved docstring and return type hints for to_datetime #42494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 32 commits into from
Jan 5, 2022
Merged

Improved docstring and return type hints for to_datetime #42494

merged 32 commits into from
Jan 5, 2022

Conversation

smarie
Copy link
Contributor

@smarie smarie commented Jul 12, 2021 •
edited by mroeschke
Loading

Improved (still not ideal) fix for #42229 (datetime parsing behaviour in case of multiple time offsets due to daylight-savings in timezones) following #42244

See also #42229 (comment)

@smarie
Update datetimes.py
bd4061c 
Following pandas-dev#42244 , improved documentation about datetime parsing.

See also pandas-dev#42229 (comment)
@smarie smarie mentioned this pull request Jul 12, 2021
Merged
4 tasks
jreback
jreback requested changes Jul 12, 2021
View reviewed changes
@jreback jreback added Docs Timezones Timezone data dtype labels Jul 12, 2021
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Aug 12, 2021
@smarie
Copy link
Contributor Author

smarie commented Aug 16, 2021

@jreback do you have any additional response to the above comment ? otherwise we can probably proceed with merging "as is"

@mroeschke mroeschke removed the Stale label Sep 2, 2021
@jreback
Copy link
Contributor

jreback commented Oct 4, 2021

closing as stale, if you want to continue working, please ping.

@jreback jreback closed this Oct 4, 2021
@smarie
Copy link
Contributor Author

smarie commented Oct 4, 2021 •
edited
Loading

@jreback thanks for the ping. I made changes in my branch but I do not see them here - I guess that you'll have to open the PR to see them ?

I tried to update the documentation of the "utc" parameter so that it is easier to understand. I added the link that @mroeschke suggested in here (rather than in a notes section, but I guess we could also do both).

Tomorrow I'll try to add a few examples showing the subtelties explained in the utc parameter doc.

@mroeschke mroeschke reopened this Oct 4, 2021
@smarie
Copy link
Contributor Author

smarie commented Oct 5, 2021

Thanks @mroeschke ! I'll push a few doc mods again and let you know.

Sylvain MARIE added 2 commits October 5, 2021 11:57
Improved overall readability by
f5cbef8 
 - adding an extended summary with details, instead of putting all details in the arguments and outputs description.
 - Fixing return type hints to add the type hints for datetime.datetime scalars or array-like.
 - adding an explicit "raises" section
 - adding illustrative examples
minor improvement
866bdcb
@smarie
Copy link
Contributor Author

smarie commented Oct 5, 2021 •
edited
Loading

@jreback and @mroeschke : all set.

Instead of trying to squeeze a complex explanation in the arguments and outputs definitions, I rather created a proper extended summary.

I also fixed the return type hints and added several examples. These examples show that the current behaviour is quite complex, but at least now users can see it :) . Let me know what you think

@smarie smarie changed the title Improved docstring for to_datetime Improved docstring and return type hints for to_datetime Oct 5, 2021
@mroeschke
Copy link
Member

Looks like the CI / Checks failed (which will check this docstring). Also recommend to keep type hinting in a separate PR so limit the review scope.

@smarie
Copy link
Contributor Author

smarie commented Dec 18, 2021 •
edited
Loading

I also realize that I did not write an entry in what's new. I see in 1.1.0 what's new that "clarified documentation" can be inserted in the bugfix section (see https://pandas.pydata.org/docs/whatsnew/v1.1.0.html?highlight=clarified#missing), so added something similar, let me know.

mroeschke
mroeschke reviewed Dec 18, 2021
View reviewed changes
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.

Looks pretty good! Off to @jreback

@mroeschke mroeschke added this to the 1.4 milestone Dec 18, 2021
jreback
jreback requested changes Dec 20, 2021
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

@smarie looks ok to me, if you can remove the whatsnew new and ping on green

@jreback
Copy link
Contributor

jreback commented Dec 23, 2021

@smarie if you can update and merge master

@smarie
Copy link
Contributor Author

smarie commented Jan 3, 2022

Sorry for the delay @jreback @mroeschke . I updated the branch and removed the whats new entry.
Build seems ok except for a few that seem unrelated to this PR

@jreback
Copy link
Contributor

jreback commented Jan 3, 2022

thanks @smarie if you can rebase one more time pls.

also if you can go over the text again and be consistent about using backticks vs class refs (e.g. i think we prefer to use :class:`Timestamp` over ``Timestamp`` (and same for datetime.datetime); you'l likely have to render this locally to make sure things come out ok. (this is also ok as a followup, but since need rebase anyhow....)

@jreback
Copy link
Contributor

jreback commented Jan 3, 2022

cc @mroeschke if comments

mroeschke
mroeschke approved these changes Jan 3, 2022
View reviewed changes
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.

LGTM after @jreback's comments about the rendering consistency. CI failure will probably be unrelated.

@jreback
Copy link
Contributor

jreback commented Jan 4, 2022

from the doc build:

/home/runner/work/pandas/pandas/pandas/core/tools/datetimes.py:docstring of pandas.core.tools.datetimes.to_datetime:72: WARNING: Unexpected indentation.

smarie
smarie commented Jan 4, 2022
View reviewed changes
@smarie
Copy link
Contributor Author

smarie commented Jan 4, 2022 •
edited
Loading

Thanks, It seems that this is now fixed @jreback . I fixed as much as possible the various sphinx roles everywhere, using explicit :class:, :mod: or :const: whenever it was relevant.

@smarie
Copy link
Contributor Author

smarie commented Jan 4, 2022

The remaining error is "/home/runner/work/pandas/pandas/pandas/core/ops/array_ops.py
/home/runner/work/pandas/pandas/pandas/core/ops/array_ops.py:98:17 - error: Unnecessary "cast" call; type is already "dtype[Unknown]" (reportUnnecessaryCast)"

so it seems unrelated

@jreback jreback merged commit 336fcc1 into pandas-dev:master Jan 5, 2022
@jreback
Copy link
Contributor

jreback commented Jan 5, 2022

thanks @smarie very nice!

@mroeschke mroeschke mentioned this pull request Jan 5, 2022
Closed
@smarie
Copy link
Contributor Author

smarie commented Jan 5, 2022

Thanks very much @jreback and @mroeschke for your patience and careful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Docs Timezones Timezone data dtype
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
3 participants
@smarie @jreback @mroeschke