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

TYP: some more static definitions of methods for DatetimeIndex #36742

Merged

Conversation

simonjayhawkins
Copy link
Member

@simonjayhawkins simonjayhawkins commented Sep 30, 2020

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Sep 30, 2020
from pandas import DataFrame, PeriodIndex, TimedeltaIndex


def dispatch_to_array_and_wrap(delegate) -> Callable[[F], F]:
Copy link
Member Author

Choose a reason for hiding this comment

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

not the final location for this. may do PeriodIndex next and move to share.

duplication with inherit_from_data. different in that inherit_from_data creates a method, whereas this wraps a method.

it maybe that we eventually replace all uses inherit_from_data and therefore can delete rather than deduplicate.

@simonjayhawkins
Copy link
Member Author

web and doc fails

2020-09-30T14:45:19.4780323Z /home/runner/work/pandas/pandas/pandas/core/indexes/datetimes.py:docstring of pandas.DatetimeIndex.rst:122: WARNING: autosummary: stub file not found 'pandas.DatetimeIndex.to_julian_date'. Check your autosummary_generate setting.
2020-09-30T14:45:19.4786910Z /home/runner/work/pandas/pandas/pandas/core/indexes/datetimes.py:docstring of pandas.DatetimeIndex.rst:122: WARNING: autosummary: stub file not found 'pandas.DatetimeIndex.isocalendar'. Check your autosummary_generate setting.


def dispatch_to_array_and_wrap(delegate) -> Callable[[F], F]:
def decorator(func: F) -> F:
name = func.__name__
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't functools.wraps already copy over the __name__ and __doc__? I think you might be able to reduce the layers of wrapping by one

Copy link
Member Author

Choose a reason for hiding this comment

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

this to copy doc from delegate not func.

wraps is to copy name (the code here does not set the name attribute) and the signature for the help/docs

can always add the doc decorator to each method individually and no need for the decorator factory. I did this originally but thought it looked messy with two decorators and repetition.

return type(self)._simple_new(arr, name=self.name)
@dispatch_to_array_and_wrap(DatetimeArray)
def isocalendar(self) -> "DataFrame":
pass
Copy link
Member

Choose a reason for hiding this comment

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

i find the 4 line version clearer than the 3 line version

Copy link
Member Author

Choose a reason for hiding this comment

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

I am happy with the existing static code, if others are happy with the repetition/duplications

@simonjayhawkins
Copy link
Member Author

Thanks @WillAyd and @jbrockmendel for the review.

I'll do two commits, one to remove the decorator factory and another to remove the actual decorator. I think the static approach that I did originally is clearer, especially considering the handling of the different return types.

I raised this PR in connection with #31160 (comment) to be more like #31160 (comment)

@simonjayhawkins
Copy link
Member Author

simonjayhawkins commented Sep 30, 2020

I think the static approach that I did originally is clearer,

and more error prone. Looks like made an error originally for to_period. but looks like can use _simple_new_ for the different return types instead of the using the Index constructor, so maybe small perf improvements.

I think adding the doc decorator to each method could also be error prone, but at the same now allows substitution so could have separate html docs for Index and array methods.

@jbrockmendel
Copy link
Member

LGTM

@jreback jreback added this to the 1.2 milestone Oct 1, 2020
@jreback jreback merged commit 1d29bf0 into pandas-dev:master Oct 1, 2020
@jreback
Copy link
Contributor

jreback commented Oct 1, 2020

sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants