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

CLN: Deprecate Index.summary (GH18217) #20028

Merged

Conversation

GGordonGordon
Copy link
Contributor

@GGordonGordon GGordonGordon commented Mar 7, 2018

@@ -2185,7 +2185,7 @@ def __getitem__(self, key):
try:
if key in self.columns and not is_mi_columns:
return self._getitem_column(key)
except:
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

either leave these (IOW the linter is not actually failing these on CI), or catch a specific exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added Exception here so that they were PEP8 compliant. (no empty except). I can change this back if we're not concerned about these failing the flake8 / PEP8 check.

Copy link
Contributor

Choose a reason for hiding this comment

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

they r not PEP compliant but we have this particular check turned off ATM because otherwise a large number of these would fail

they need fixing but generally want to catch specific exceptions rather than Exception (if we can help it)

@@ -963,6 +963,15 @@ def summary(self, name=None):
result = result.replace("'", "")
return result

def summary(self, name=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

you only need to deprecate in base class one (the sub-class is the same)

@@ -941,7 +941,7 @@ def where(self, cond, other=None):
return self._shallow_copy(result,
**self._get_attributes_dict())

def summary(self, name=None):
def _summary(self, name=None):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

can you update the doc-strings here (and in the base class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback - What specific updates were you referring to? Do the sphinx tags need to be added here?

Copy link
Contributor

Choose a reason for hiding this comment

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

don't need deprecated in the _summary doc-strings

@@ -67,6 +69,11 @@ def test_representation_to_series(self):
result = repr(pd.Series(idx))
assert result == expected

def test_summary_deprecated(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary

@jreback jreback added Output-Formatting __repr__ of pandas objects, to_string Deprecate Functionality to remove in pandas labels Mar 7, 2018
assert result == expected

def test_summary_deprecated(self):
# GH18217
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback - Should this be removed as well since the timedeltas version is being removed and it should be taken care of in the base test case?

@jreback
Copy link
Contributor

jreback commented Mar 13, 2018

can you rebase and update according to comments.

@GGordonGordon GGordonGordon force-pushed the GH18217_deprecate_index_summary branch from 5a2d31b to 9be7985 Compare March 14, 2018 01:09
@codecov
Copy link

codecov bot commented Mar 14, 2018

Codecov Report

Merging #20028 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20028      +/-   ##
==========================================
+ Coverage   91.76%   91.77%   +0.01%     
==========================================
  Files         150      152       +2     
  Lines       49151    49178      +27     
==========================================
+ Hits        45102    45133      +31     
+ Misses       4049     4045       -4
Flag Coverage Δ
#multiple 90.16% <100%> (+0.01%) ⬆️
#single 41.84% <33.33%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/frame.py 97.18% <100%> (ø) ⬆️
pandas/core/indexes/base.py 96.67% <100%> (ø) ⬆️
pandas/core/indexes/datetimelike.py 96.72% <100%> (ø) ⬆️
pandas/util/testing.py 83.74% <0%> (-0.21%) ⬇️
pandas/core/base.py 96.77% <0%> (-0.02%) ⬇️
pandas/core/window.py 96.29% <0%> (-0.02%) ⬇️
pandas/core/indexes/category.py 97.3% <0%> (-0.01%) ⬇️
pandas/core/series.py 93.84% <0%> (-0.01%) ⬇️
pandas/core/indexes/multi.py 95.05% <0%> (-0.01%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3783ccc...ccb33cb. Read the comment docs.

def summary(self, name=None):
def _summary(self, name=None):
"""
Return a summarized representation
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need a deprecated tag on the _summary methods

@@ -1404,6 +1409,16 @@ def summary(self, name=None):
name = type(self).__name__
return '%s: %s entries%s' % (name, len(self), index_summary)

def summary(self, name=None):
"""
Return a summarized representation
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a Parameters & Returns section

@@ -941,7 +941,7 @@ def where(self, cond, other=None):
return self._shallow_copy(result,
**self._get_attributes_dict())

def summary(self, name=None):
def _summary(self, name=None):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need deprecated in the _summary doc-strings

@jreback jreback added this to the 0.23.0 milestone Mar 15, 2018
@jreback
Copy link
Contributor

jreback commented Mar 15, 2018

minor comment. ping when pushed.

"""
Return a summarized representation
.. deprecated:: 0.23.0
No longer supported
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the 'No longer supported'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback - This has been removed per your request. Update has been pushed.

@jreback jreback merged commit bd70e75 into pandas-dev:master Mar 16, 2018
@jreback
Copy link
Contributor

jreback commented Mar 16, 2018

thanks @GGordonGordon

@GGordonGordon GGordonGordon deleted the GH18217_deprecate_index_summary branch March 17, 2018 23:52
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 Output-Formatting __repr__ of pandas objects, to_string
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Index.summary
3 participants