10000 CLN: Move some PI/DTI methods to EA subclasses, implement tests by jbrockmendel · Pull Request #22961 · pandas-dev/pandas · GitHub
[go: up one dir, main page]
Skip to content

CLN: Move some PI/DTI methods to EA subclasses, implement tests #22961

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

Merged
merged 11 commits into from
Oct 8, 2018
Merged

CLN: Move some PI/DTI methods to EA subclasses, implement tests #22961

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0a5f343
Move some PeriodIndex/DatetimeIndex methods to EA subclasses, impleme…
jbrockmendel Oct 3, 2018
f1f5146
fixup missing import
jbrockmendel Oct 3, 2018
effef65
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 3, 2018
4eb5060
Suggested renames
jbrockmendel Oct 3, 2018
09f1b02
Revert change that broke things
jbrockmendel Oct 3, 2018
68116c4
Revert change that broke tests
jbrockmendel Oct 3, 2018
12eb1b9
Fix failing test by ammending constructor
jbrockmendel Oct 3, 2018
ed30d73
flake8 whitespace fixup
jbrockmendel Oct 3, 2018
5b4c828
requested verbosification
jbrockmendel Oct 4, 2018
61808d3
fixup property wrapping
jbrockmendel Oct 4, 2018
927e4bd
Merge branch 'master' of https://github.com/pandas-dev/pandas into dl…
jbrockmendel Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup property wrapping
  • Loading branch information
@jbrockmendel
jbrockmendel committed Oct 4, 2018
commit 61808d3a2b0f650feca5c6546bd8b41208d4c7bd
4 changes: 2 additions & 2 deletions pandas/core/indexes/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,12 +449,12 @@ def is_full(self):
@property
@Appender(PeriodArrayMixin.start_time.__doc__)
def start_time(self):
return PeriodArrayMixin.start_time(self)
return PeriodArrayMixin.start_time.fget(self)

@property
@Appender(PeriodArrayMixin.end_time.__doc__)
def end_time(self):
return PeriodArrayMixin.end_time(self)
return PeriodArrayMixin.end_time.fget(self)

Copy link
Member

Choose a reason for hiding this comment

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

I would leave those properties here, since we will need to add them back when doing the actual refactor

(or you can call the ArrayMixin version like you did for to_period/to_timestamp, but in this case that will be more verbose than what is there now (but which is not necessarily a problem, we can simplify when doing the actual refactor))

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so on this PR, these are inherited from DatetimelikeArrayMixin, which won't be a parent of PeriodIndex in the future?

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 think in the future, assuming we do away with inheritance (which I'm still -0 on), we'll use something akin to

@inherits_from_array(["start_time", "end_time", ...]
class PeriodIndex(...):

to succinctly implement appropriate wrapping.

In the interim, removing code from the Index subclasses makes it easier for me to sort out what methods still need to be migrated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so on this PR, these are inherited from DatetimelikeArrayMixin

Close, they are inherited from PeriodArrayMixin

Copy link
Member

Choose a reason for hiding this comment

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

I think in the future, we'll use something akin to

Yes, that might certainly be (or maybe with a method adding them like we do for eg arithmetic methods), but I don't think we will do that in an initial PR that does the refactor, so moving it away now, will make that PR bigger / less easy to understand

In the interim, removing code from the Index subclasses makes it easier for me to sort out what methods still need to be migrated.

Then I would suggest calling the ArrayMixin implementation as you did for to_timestamp/to_array. That also makes it clear what is already moved and what not yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually given the CI turnaround, if there’s consensus here I’d like to make this change in a follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

I would do it here (travis is still fast)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

def _mpl_repr(self):
# how to represent ourselves to matplotlib
Expand Down
0