-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
DEPR: deprecate notebook argument in DataFrame.to_html() #26815
DEPR: deprecate notebook argument in DataFrame.to_html() #26815
Conversation
Code looks pretty good. Just to clarify since I'm not super familiar with this one, does this serve any purpose from an end user perspective or is it pure cruft? Just wondering if we should document the alternatives to this if any |
I'm not sure of the history. Assuming that it was needed internally but added to a public method. So I don't see it serving any purpose to an end user.
The alternative is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the deprecation here: #6581
doc/source/whatsnew/v0.25.0.rst
Outdated
@@ -495,7 +495,7 @@ Other Deprecations | |||
Use the public attributes :attr:`~RangeIndex.start`, :attr:`~RangeIndex.stop` and :attr:`~RangeIndex.step` instead (:issue:`26581`). | |||
- The :meth:`Series.ftype`, :meth:`Series.ftypes` and :meth:`DataFrame.ftypes` methods are deprecated and will be removed in a future version. | |||
Instead, use :meth:`Series.dtype` and :meth:`DataFrame.dtypes` (:issue:`26705`). | |||
|
|||
- The ``notebook`` argument of :meth:`DataFrame.to_html` is deprecated (:issue:`23820`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention the replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there would be no replacement from an end user perspective. see #26815 (comment)
pandas/core/frame.py
Outdated
@@ -650,8 +650,11 @@ def _repr_html_(self): | |||
max_cols = get_option("display.max_columns") | |||
show_dimensions = get_option("display.show_dimensions") | |||
|
|||
return self.to_html(max_rows=max_rows, max_cols=max_cols, | |||
show_dimensions=show_dimensions, notebook=True) | |||
formatter = fmt.DataFrameFormatter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_repr_html_
just calls to_html
with notebook=True
, so need to avoid the FutureWarning in the notebook.
eventually i'd like to have _repr_html_
instantiate NotebookFormatter
directly and to_html
instantiate HTMLFormatter
directly. But at the moment neither are subclasses of DataFrameFormatter
pandas/core/frame.py
Outdated
@@ -2158,6 +2162,9 @@ def to_html(self, buf=None, columns=None, col_space=None, header=True, | |||
Convert the characters <, >, and & to HTML-safe sequences. | |||
notebook : {True, False}, default False | |||
Whether the generated HTML is for IPython Notebook. | |||
|
|||
.. deprecated:: 0.25.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there would be no replacement from an end user perspective. see #26815 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update this to 1.0.0
Codecov Report
@@ Coverage Diff @@
## master #26815 +/- ##
==========================================
- Coverage 91.86% 90.45% -1.42%
==========================================
Files 179 179
Lines 50707 50710 +3
==========================================
- Hits 46584 45870 -714
- Misses 4123 4840 +717
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26815 +/- ##
=========================================
Coverage ? 91.86%
=========================================
Files ? 180
Lines ? 50719
Branches ? 0
=========================================
Hits ? 46593
Misses ? 4126
Partials ? 0
Continue to review full report at Codecov.
|
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment otherwise lgtm
pandas/core/frame.py
Outdated
@@ -2158,6 +2162,9 @@ def to_html(self, buf=None, columns=None, col_space=None, header=True, | |||
Convert the characters <, >, and & to HTML-safe sequences. | |||
notebook : {True, False}, default False | |||
Whether the generated HTML is for IPython Notebook. | |||
|
|||
.. deprecated:: 0.25.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update this to 1.0.0
General question on this - are we adding deprecations to the 1.0.0 release or removing outright? |
I thought adding deprecations. May be wrong though.
…On Fri, Aug 30, 2019 at 9:42 AM William Ayd ***@***.***> wrote:
General question on this - are we adding deprecations to the 1.0.0 release
or removing outright?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#26815?email_source=notifications&email_token=AAKAOITUQYS56WHE4KJFJT3QHEWT5A5CNFSM4HXNVKSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5R3JRI#issuecomment-526628037>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIWJS5BOXZNVFF7I4OLQHEWT5ANCNFSM4HXNVKSA>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm @TomAugspurger
This deprecation docs / warning should have some indication for what users should do instead. What do we recommend? |
there would be no replacement from an end user perspective. see #26815 (comment) |
So you're saying it's not useful to users? How do you know that?
Why don't we document what `notebook=True` used to imply? Or, alternatively,
we could just say `_repr_html_` is part of the API for this purpose. It's a
well established
interface that we aren't going to remove.
…On Fri, Sep 6, 2019 at 3:20 AM Simon Hawkins ***@***.***> wrote:
This deprecation docs / warning should have some indication for what users
should do instead. What do we recommend?
there would be no replacement from an end user perspective. see #26815
(comment)
<#26815 (comment)>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26815?email_source=notifications&email_token=AAKAOIWZXQI3LAV3AFES7G3QIIHEFA5CNFSM4HXNVKSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6CD7AY#issuecomment-528760707>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIWXDTAIVOAO3L6WSY3QIIHEFANCNFSM4HXNVKSA>
.
|
@simonjayhawkins can you summarize status here |
there is not agreement that this should be deprecated. so will close and maybe reopen in the future if there is a stronger argument for deprecation. |
git diff upstream/master -u -- "*.py" | flake8 --diff