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

API: Add various deprecated attributes to ._deprecated #28805

Merged
merged 2 commits into from
Oct 12, 2019

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Oct 6, 2019

This add various deprecated attributes to their respective types' _deprecated. The effect is that these are now also hidden when tab-completing, making navigating the pandas attributes easier.

@topper-123 topper-123 force-pushed the hide_deprecated_in_ipython branch from 41d014f to a4f6744 Compare October 6, 2019 08:42
@@ -331,7 +331,7 @@ class Categorical(ExtensionArray, PandasObject):
__array_priority__ = 1000
_dtype = CategoricalDtype(ordered=False)
# tolist is not actually deprecated, just suppressed in the __dir__
_deprecations = frozenset(["labels", "tolist"])
_deprecations = frozenset(["get_values", "tolist"])
Copy link
Contributor Author

@topper-123 topper-123 Oct 6, 2019

Choose a reason for hiding this comment

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

labels is no longer an atribute on Categorical.

@topper-123 topper-123 force-pushed the hide_deprecated_in_ipython branch 2 times, most recently from 9e67b09 to c729d85 Compare October 6, 2019 09:10
@jreback jreback added the Deprecate Functionality to remove in pandas label Oct 6, 2019
@jreback jreback added this to the 1.0 milestone Oct 6, 2019
@jreback
Copy link
Contributor

jreback commented Oct 6, 2019

seem fine. Any way to have a systematic test here? I don't necessarly care about the actual tab completion itself (though that would be nice), rather that the list is checked against actual deprecations. An idea: we could parse our doc source for Deprecated and then use that as the source of truth (also could do the same with tests).

@topper-123
Copy link
Contributor Author

Yeah, I thought about it, but can't think of a method that doesn't parse all the source files, which would be inefficient. Maybe add a deprecated decorator, where instead of manually adding to _deprecations, we'd do:

class AClass:
    @deprecated(hide=True)
    def (self, ...):
        ...

But I'm however not personally particularly interested in implementing this, and it should probably be something that is implemented as an independent project outside of pandas anyway...

@topper-123 topper-123 force-pushed the hide_deprecated_in_ipython branch from c729d85 to b3705bc Compare October 7, 2019 16:49
@WillAyd
Copy link
Member

WillAyd commented Oct 7, 2019

Could maybe leverage something similar to what I posted in #27353 for AST parsing and check where FutureWarning gets raised, but yea also agree can be done separately

@WillAyd
Copy link
Member

WillAyd commented Oct 7, 2019

Hmm actually it would be nice to have something like that as a test here as well otherwise I can see this getting out of sync very fast

@topper-123
Copy link
Contributor Author

Yeah, I'd like this to exist, I'm just not interested in coding it up myself :-).

@WillAyd
Copy link
Member

WillAyd commented Oct 7, 2019

I guess I'd prefer not to do something like this without checks and balances (whether a test or a code check of some sort). Especially if exposing to end users via whatsnew it could add technical debt

@jreback
Copy link
Contributor

jreback commented Oct 8, 2019

@topper-123 can you give a manual check in ipython to see if everything is working? I agree an automated test would be nice, but not going to block merging this as its a net improvement.

@topper-123 topper-123 force-pushed the hide_deprecated_in_ipython branch from b3705bc to c1b21cb Compare October 8, 2019 14:55
@topper-123
Copy link
Contributor Author

topper-123 commented Oct 8, 2019

I've rechecked, all is ok. I found "reshape" in Series._deprecations, which isn't a method on Series, so I removed it.

The oldest deprecated name I found was NDFrame.ix, which was deprecated in v0.20, so could be removed now.

@topper-123
Copy link
Contributor Author

I notice I said I was not interested above, which could be seen as a bit negative.

What I meant was that it seems a bit big of a undertaking relative to the benefits IMO, so I don't feel it is worth it to spend time on, relative to other PR...If someone else would like to take it on, I'd be positive to the change.

@WillAyd
Copy link
Member

WillAyd commented Oct 8, 2019

I notice I said I was not interested above, which could be seen as a bit negative.

Ha all good - I understood what you were trying to say. I've flip flopped on the issue so also not a blocker for me, just figured was worth thinking twice about

@topper-123
Copy link
Contributor Author

Is this ok?

@jorisvandenbossche
Copy link
Member

Looks good to me. I agree that a full blown test is not worth to block this.

I was just thinking, an easy test to keep a bit track of this might be to thest that all elements in _deprecations actually exist on the object (so we ensure to remove them when we remove the property/method). Not that things break if we forget to remove a name, but that could ensure we keep this list clean.

@topper-123
Copy link
Contributor Author

topper-123 commented Oct 12, 2019

That's a a very small win IMO, because if the attribute doesn't exist, there's not really any loss from the string existing in _deprecations, just a bit untidy. The big wins would be:

  • know which deprecated attributed show/should show up in the dir
  • know if a not-deprecated attribute is accidentally hidden by its name being added to the _deprecations.

Neither which are trivial to test for IMO.

@jreback jreback merged commit 1e92886 into pandas-dev:master Oct 12, 2019
@jreback
Copy link
Contributor

jreback commented Oct 12, 2019

thanks @topper-123

ok we will have to be vigilant to add things when they are deprecated.

@topper-123 topper-123 deleted the hide_deprecated_in_ipython branch October 12, 2019 19:59
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants