-
-
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
Add option for DataFrame.to_html() to render URL data as links (#2679) #23715
Add option for DataFrame.to_html() to render URL data as links (#2679) #23715
Conversation
Hello @benarthur91! Thanks for updating the PR.
Comment last updated on November 26, 2018 at 05:41 Hours UTC |
@benarthur91 : This is looking good! Next up: could you resolve the merge conflicts? |
Thanks for the feedback everyone, I've pushed the requested changes. I'm unfamiliar with the PR workflow, so please let me know if I've done anything incorrectly. Edit: Thanks @gfyoung, I'm assuming this means rebase onto master, resolve conflicts, and force push that branch over the current? |
Yep, that's correct. |
Typically preferred if you merge in the latest master instead of rebasing. Keeps github comment history and doesn’t require a force push |
974908d
to
0702824
Compare
Gotcha, sorry about that. I still have the pre-rebase branch with original commits, would it be helpful if I pushed that one up instead? |
@benarthur91 : Just leave it as is and work off what you just pushed. It's fine. |
Does anyone know what might have happened? Getting a seemingly unrelated error in pandas/tests/plotting/test_datetimelike.py, I couldn't see any obvious reason that would have happened. |
Might be a weird Travis thing. Don't forget to fix the |
Ah, I didn't see a clear error message and assumed it was related to the other failure. As you mention isort, I'm now thinking it's either a) imports are in the wrong order or b) it doesn't like that I'm importing a private method? Where can I look to find the proper order of imports? Also I ran flake8 on frame.py locally and it reported no issues (even when I changed the order of imports), would you expect it to? |
@benarthur91 : You need to install |
Pretty neat :) Thanks for clarifying, I just pushed that change. |
Codecov Report
@@ Coverage Diff @@
## master #23715 +/- ##
==========================================
+ Coverage 92.22% 92.22% +<.01%
==========================================
Files 162 162
Lines 51777 51785 +8
==========================================
+ Hits 47751 47759 +8
Misses 4026 4026
Continue to review full report at Codecov.
|
Is there anything else I should do to clean this up right now? |
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 comments otherwise lgtm
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.
can you add a whatsnew note, New Features is prob good. Also we have some docs on writing .to_html()
in io.rst
, can you add an example of this there (with a versionadded tag)
Added note to whatsnew and also a short description and example in io.rst I also checked the last two boxes in the original (autogenerated) comment - I'm assuming that's for me to do and not someone else, but not sure. Let me know if there's anything else I can do. |
}, | ||
] | ||
df = DataFrame(data, columns=['foo', 'bar', None], | ||
index=range(len(data))) |
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.
You don't need to pass index
here as this is the default behavior
.. ipython:: python | ||
:suppress: | ||
|
||
write_html(url_df, 'render_links', render_links=True) |
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.
what is this for?
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.
This runs the write_html() call without including its input or output in the notebook cells that appear in the doc. This makes available the static file rendered on line 2625.
I copied a pattern used elsewhere in this file, example "bold_rows" section, starting line 2584.
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.
What's the downside of just printing it? I think creating files like this is hard to keep track of within our codebase so less than ideal
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.
The generated HTML is also printed in a notebook cell. The lines pointed out by @jreback write that HTML to a separate file that can be rendered.
Here's what it looks like after building:
That said, if having the rendered sample is not worth the overhead of the extra file, I'm happy to remove.
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.
hmm i am not sure we actually need to show the html, showing the rendered version is enough. but since we are doing it for all of these, ok
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.
just some small comments. ping on green.
pandas/core/frame.py
Outdated
@@ -2067,6 +2067,12 @@ def to_html(self, buf=None, columns=None, col_space=None, header=True, | |||
A css id is included in the opening `<table>` tag if specified. | |||
|
|||
.. versionadded:: 0.23.0 | |||
|
|||
render_links : boolean, default False |
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.
i think our standard is for bool, @datapythonista should probably validate this generally as I seem mixed usage all over the place.
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.
yes, we still need to fix many cases before we can validate this, but if you run ./scripts/validate_docstrings.py pandas.DataFrame.to_html
it will report this, and any other formatting issue.
<td>pydata.org</td> | ||
</tr> | ||
</tbody> | ||
</table> |
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.
can you add a linefeed here (and on the one below)
doc/source/whatsnew/v0.24.0.rst
Outdated
@@ -27,6 +27,7 @@ New features | |||
- :meth:`DataFrame.corr` and :meth:`Series.corr` now accept a callable for generic calculation methods of correlation, e.g. histogram intersection (:issue:`22684`) | |||
- :func:`DataFrame.to_string` now accepts ``decimal`` as an argument, allowing the user to specify which decimal separator should be used in the output. (:issue:`23614`) | |||
- :func:`DataFrame.read_feather` now accepts ``columns`` as an argument, allowing the user to specify which columns should be read. (:issue:`24025`) | |||
- :func:`DataFrame.to_html` now accepts ``render_links`` as an argument, allowing the user to generate HTML with links to any URLs that appear in the DataFrame. (:issue:`2679`) |
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.
can you add a ref to the new docs that you added.
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 a link here or just a mention of the addition to docs?
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.
If you add a link I can approve and merge
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 point, pls ping on green. @WillAyd over to you. |
Just got in front of keyboard and was about to ping you 😃 thanks for approving, will wait to hear from @WillAyd. |
Thanks @benarthur91 - nice change! |
Thanks to everyone for all the help! |
git diff upstream/master -u -- "*.py" | flake8 --diff
Two lines were added to tests that exceed the 79 character limit.