-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: update the Period.dayofweek attribute docstring #20280
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: update the Period.dayofweek attribute docstring #20280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the highlighted issues?
pandas/_libs/tslibs/period.pyx
Outdated
Return the day of the week. | ||
|
||
This attribute returns the day of the week on which the particular | ||
date occurs with Monady=0, Sunday=6. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Monday
pandas/_libs/tslibs/period.pyx
Outdated
|
||
Examples | ||
-------- | ||
>>> p=pd.Period('2012-1-1 19:00', freq='H') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add spaces around
p
to follow flake8 also in docstrings. - Perhaps better to use
period
for the variable name, it's short enough.
""" | ||
Return the day of the week. | ||
|
||
This attribute returns the day of the week on which the particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, this will return the day of the week on which the particular starting date for the given period occurs. I think this is important to mention, because at first I was confused thinking "how a period of one month can have a day of the week?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have this discussion in another PR as well (https://github.com/pandas-dev/pandas/pull/20277/files#r173651389). Only, it is not necessarily the starting date, it depends on the freq
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue is here: #20324.
pandas/_libs/tslibs/period.pyx
Outdated
|
||
Returns | ||
------- | ||
Int:Range of 0 to 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not following the style guide, see https://python-sprints.github.io/pandas/guide/pandas_docstring.html#section-4-returns-or-yields
pandas/_libs/tslibs/period.pyx
Outdated
Returns | ||
------- | ||
Int | ||
Range of 0 to 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'd say correct English should be "Range from 0 to 6". So "from", not "of". But I'm not a native English speaker so please double check.
- Perhaps add (included) to make clear 6 is also included. Since
range(0, 6)
in Python doesn't include the 6. :) - End line with period.
pandas/_libs/tslibs/period.pyx
Outdated
Returns | ||
------- | ||
Int | ||
Range from 0 to 6(included). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space of 6
""" | ||
Return the day of the week. | ||
|
||
This attribute returns the day of the week on which the particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have this discussion in another PR as well (https://github.com/pandas-dev/pandas/pull/20277/files#r173651389). Only, it is not necessarily the starting date, it depends on the freq
.
pandas/_libs/tslibs/period.pyx
Outdated
See also | ||
-------- | ||
Period.dayofyear | ||
Return the day of year. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this in form of Period.dayofyear : Return the day of year.
? (same for one below)
Codecov Report
@@ Coverage Diff @@
## master #20280 +/- ##
=========================================
Coverage ? 91.7%
=========================================
Files ? 150
Lines ? 49168
Branches ? 0
=========================================
Hits ? 45090
Misses ? 4078
Partials ? 0
Continue to review full report at Codecov.
|
@hammadmashkoor Thanks a lot! |
Thanks @jorisvandenbossche |
Checklist for the pandas documentation sprint (ignore this if you are doing
an unrelated PR):
scripts/validate_docstrings.py <your-function-or-method>
git diff upstream/master -u -- "*.py" | flake8 --diff
python doc/make.py --single <your-function-or-method>
Please include the output of the validation script below between the "```" ticks:
If the validation script still gives errors, but you think there is a good reason
to deviate in this case (and there are certainly such cases), please state this
explicitly.