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.

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

DOC: fix punctuation in Timestamp/Timedelta docstrings #28053

Merged
merged 10 commits into from
Sep 28, 2019
Merged

DOC: fix punctuation in Timestamp/Timedelta docstrings #28053

merged 10 commits into from
Sep 28, 2019

Conversation

sparalic
Copy link
Contributor

@sparalic sparalic commented Aug 21, 2019 •
edited
Loading

Add periods to Timestamp and Timedelta doctrings for the following attributes and methods:

pandas.Timestamp.resolution
pandas.Timestamp.tz
pandas.Timestamp.ceil
pandas.Timestamp.combine
pandas.Timestamp.floor
pandas.Timestamp.fromordinal
pandas.Timestamp.replace
pandas.Timestamp.round
pandas.Timestamp.weekday
pandas.Timedelta.ceil
pandas.Timedelta.floor
pandas.Timedelta.isoformat
pandas.Timedelta.round

I did not find the method for pandas.Timestamp.isoweekday this in the Timestamp source code. Was this deprecated ? @datapythonista

datapythonista
datapythonista approved these changes Aug 21, 2019
View reviewed changes
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for working on this @sparalic.

Can you update your branch with upstream (i.e. git fetch up && git merge upstream/master %% git push) to rerun the CI, and see if it finishes green this time. Thanks!

jbrockmendel
jbrockmendel reviewed Aug 21, 2019
View reviewed changes
pandas/_libs/tslibs/timedeltas.pyx
@@ -1150,7 +1150,7 @@ cdef class _Timedelta(timedelta):
"""
Format Timedelta as ISO 8601 Duration like
``P[n]Y[n]M[n]DT[n]H[n]M[n]S``, where the ``[n]`` s are replaced by the
values. See https://en.wikipedia.org/wiki/ISO_8601#Durations
values. See https://en.wikipedia.org/wiki/ISO_8601#Durations.
Copy link
Member

Choose a reason for hiding this comment

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

@datapythonista I'm never sure what to do with URLs at the end of a sentence. Has this been discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbrockmendel I wasn't sure either, but this seem to have trigger the error for the Timedelta.isoformat docstring.

@datapythonista updated my branch and the CI still failed.

Copy link
Member

Choose a reason for hiding this comment

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

@sparalic can you build the html page of that docstring? IIRC you can build that page only with ./doc/make.py --single=pandas.Timedelta.isoformat (building all the docs takes some time). This way we can see if the dot is added to the url link and breaks it, or if Sphinx is smart enough and does what we want.

Looks like there are tests that check those docstrings, you'll have to update them to pass the CI. Let me know if you need help finding the errors in the CI logs...

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@datapythonista I was able to sucessfylly build the html page for Timedelta.isoformat without any warnings. So it seems like Sphinx was smart enough to ignore the period on the end.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open it with a browser, and paste here a screenshot? I don't think Sphinx will warn, it could just create a link with the period in it, that is broken.

Copy link
Member

Choose a reason for hiding this comment

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

The link is created correctly.

@WillAyd WillAyd added this to the 1.0 milestone Aug 26, 2019
@WillAyd
Copy link
Member

WillAyd commented Aug 26, 2019

Looks like this is failing test_nat_doc_strings in pandas/tests/scalar/test_nat.py - @sparalic can you take a look at that and update accordingly?

@pandas-dev pandas-dev deleted a comment Aug 27, 2019
@sparalic
Copy link
Contributor Author

sparalic commented Aug 31, 2019 •
edited
Loading

@WillAyd I am not sure what's happening now. I fixed the errors associated is mismatching of overlapping methods per the CI logs . However, it still seems to be failing.

@jorisvandenbossche
Copy link
Member

@sparalic the tests are failing because we have apparently tests that check that the docstring of Timestamp.some_method and NaT.some_method is the same (to ensure they keep being consistent).
So you will need to make the same changes (adding the periods) for NaT as well. You can find those in pandas/_libs/tslibs/nattype.pyx

@jorisvandenbossche
Copy link
Member

@sparalic see my last comment about how to get this PR passing CI, if you have some time to update it, that would be welcome!

@sparalic
Copy link
Contributor Author

@jorisvandenbossche I am sorry for the delay. I found the culprits and updated them all. I think it should be all set now.

jorisvandenbossche
jorisvandenbossche approved these changes Sep 28, 2019
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update! Now all passing CI

@jorisvandenbossche jorisvandenbossche changed the title DOC:Timestamp timedelta DOC: fix punctuation in Timestamp/Timedelta docstrings Sep 28, 2019
@jorisvandenbossche jorisvandenbossche merged commit 0e97074 into pandas-dev:master Sep 28, 2019
This was referenced Nov 2, 2019
Closed
Merged
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@sparalic @proost
DOC: fix punctuation in Timestamp/Timedelta docstrings (pandas-dev#28053
4d452b0 
)
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@sparalic @proost
DOC: fix punctuation in Timestamp/Timedelta docstrings (pandas-dev#28053
d224d07 
)
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
@sparalic
DOC: fix punctuation in Timestamp/Timedelta docstrings (pandas-dev#28053
e4a98ae 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datapythonista datapythonista datapythonista approved these changes

@jbrockmendel jbrockmendel jbrockmendel left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sparalic @WillAyd @jorisvandenbossche @jbrockmendel @datapythonista @simonjayhawkins