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

CLN: Deprecate dayofweek/hello day_of_week (#9606) #37390

Merged
merged 10 commits into from
Oct 29, 2020
Merged

CLN: Deprecate dayofweek/hello day_of_week (#9606) #37390

merged 10 commits into from
Oct 29, 2020

Conversation

abhishekmangla
Copy link
Contributor

@abhishekmangla abhishekmangla commented Oct 24, 2020 •
edited
Loading

Rename dayofweek properties in Period, Datetime, and PeriodArray classes.
Add day_of_week property to respective tests for each class.

arw2019
arw2019 suggested changes Oct 24, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 left a comment

Choose a reason for hiding this comment

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

Thanks @abhishekmangla for the PR!

Looks good, some comments.

You'll also need to add a whatsnew (v1.2) under enhancements

Let us know if you have any questions!

pandas/_libs/tslibs/timestamps.pyx Show resolved Hide resolved
pandas/_libs/tslibs/period.pyx Outdated Show resolved Hide resolved
pandas/tests/generic/test_finalize.py Outdated Show resolved Hide resolved
pandas/tests/indexes/datetimes/test_scalar_compat.py Outdated Show resolved Hide resolved
pandas/tests/indexes/period/test_period.py Outdated Show resolved Hide resolved
@abhishekmangla
Copy link
Contributor Author

@arw2019 Thank you for the review. I addressed your comments and added an entry to whatsnew file.

arw2019
arw2019 suggested changes Oct 25, 2020
View reviewed changes
Copy link
Member

@arw2019 arw2019 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 following up on this!

Some more (minor) comments, otherwise good to go IMO

doc/source/whatsnew/v1.2.0.rst Outdated Show resolved Hide resolved
pandas/_libs/tslibs/period.pyx Outdated Show resolved Hide resolved
pandas/_libs/tslibs/timestamps.pyx Outdated Show resolved Hide resolved
@abhishekmangla
Copy link
Contributor Author

abhishekmangla commented Oct 25, 2020 •
edited
Loading

@arw2019 Resolved minor issues. Do you know how to locally test .rst files? In the CI stage of the checks, I am failing things like
/home/runner/work/pandas/pandas/pandas/core/indexes/period.py:docstring of pandas.PeriodIndex.rst:77: WARNING: autosummary: stub file not found 'pandas.PeriodIndex.day_of_year'. Check your autosummary_generate setting. 2391
AND
/home/runner/work/pandas/pandas/doc/source/whatsnew/v1.2.0.rst:31: WARNING: Explicit markup ends without a blank line; unexpected unindent.

./ci/code_checks.sh shows no errors locally

@arw2019
Copy link
Member

arw2019 commented Oct 25, 2020

@arw2019 Resolved minor issues. Do you know how to locally test .rst files? In the CI stage of the checks, I am failing things like
/home/runner/work/pandas/pandas/pandas/core/indexes/period.py:docstring of pandas.PeriodIndex.rst:77: WARNING: autosummary: stub file not found 'pandas.PeriodIndex.day_of_year'. Check your autosummary_generate setting. 2391

I think you (maybe) need to add the new methods to the rst files manually - for example in doc/source/reference/series.rst both daysinmonth and days_in_month are listed

/home/runner/work/pandas/pandas/doc/source/whatsnew/v1.2.0.rst:31: WARNING: Explicit markup ends without a blank line; unexpected unindent.

This one is a linting issue

@abhishekmangla
CLN: Deprecate dayofweek/hello day_of_week (pandas-dev#9606)
b0cba8e 
Rename dayofweek properties in Period, Datetime, and PeriodArray classes.
Add day_of_week property to respective tests for each class.
@abhishekmangla
Test precommit hook
65595fb
@abhishekmangla
Retest precommit
e65e7b6
@abhishekmangla
Fix whitespace
2aab957
@abhishekmangla
CLN: Resolve comments from @arw2019
e1684b6 
Alias dep. function instead of extra property
Remove comments from test files
@abhishekmangla
Add proposed change to whatsnew
9320a7d
@abhishekmangla
ENH: Support day_of_year over dayofyear (pandas-dev#9606)
a4b88df 
Similar to changes for day_of_week, create aliases for day_of_year and add to respective test files to ensure back-compat.
Also fix whatsnew rst file
@abhishekmangla
Add autogenerated doc references
0edff54
@abhishekmangla
Fix newline error Sphinx in whatsnew doc
84be52f
@abhishekmangla
Copy link
Contributor Author

@arw2019 The only unit test that is failing on Azure pipeline is test_binary_arith_ops which seems to be a known unreliable test (#37328). (The test suite passes on my macOS machine)

Is this good to merge now?

@jreback jreback added this to the 1.2 milestone Oct 26, 2020
jreback
jreback requested changes Oct 26, 2020
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.

this is fine. ping on green.

doc/source/whatsnew/v1.2.0.rst Outdated
@@ -13,14 +13,23 @@ including other versions of pandas.
Enhancements
~~~~~~~~~~~~



.. _whatsnew_120.improve_timeseries_accessor_naming:
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just put these notes in Other Enhancements section

doc/source/whatsnew/v1.2.0.rst Outdated
:class:`Series` and :class:`DataFrame` can now be created with ``allows_duplicate_labels=False`` flag to
control whether the index or columns can contain duplicate labels (:issue:`28394`). This can be used to
prevent accidental introduction of duplicate labels, which can affect downstream operations.
:class:`Series` and :class:`DataFrame` can now be created with ``allows_duplicate_labels=False`` flag to control whether the index or columns can contain duplicate labels (:issue:`28394`). This can be used to prevent accidental introduction of duplicate labels, which can affect downstream operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

pls don't adjust other notes

@jreback
Copy link
Contributor

jreback commented Oct 26, 2020

@pandas-dev/pandas-core should we do a DeprecationWarning / FutureWarning on the original attributes? (would be a followon)

@abhishekmangla
Copy link
Contributor Author

@jreback All checks green except aforementioned test_binary_arith_ops

@jreback jreback merged commit 10e5ad7 into pandas-dev:master Oct 29, 2020
@jreback
Copy link
Contributor

jreback commented Oct 29, 2020

thanks @abhishekmangla very nice

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
@abhishekmangla
CLN: Deprecate dayofweek/hello day_of_week (pandas-dev#9606) (pandas-…
de44821 
…dev#37390)
ukarroum pushed a commit to ukarroum/pandas that referenced this pull request Nov 2, 2020
@abhishekmangla @ukarroum
CLN: Deprecate dayofweek/hello day_of_week (pandas-dev#9606) (pandas-…
8c1eded 
…dev#37390)
@simonjayhawkins simonjayhawkins mentioned this pull request Apr 13, 2022
Merged
@Dr-Irv Dr-Irv mentioned this pull request Apr 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback requested changes

@arw2019 arw2019 arw2019 requested changes

Assignees
No one assigned
Labels
API Design Frequency DateOffsets
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

API: timeseries accessors naming convention
3 participants
@abhishekmangla @arw2019 @jreback