-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
To html encoding add #28692
To html encoding add #28692
Conversation
hi @josibake , I found two checks failed but I just got |
hey @ailurus1991 , if you look further up in the logs, it says that running try running |
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 test for this - typically the first thing we would look at in review
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.
Thanks. Looks pretty good some minor questions. Can you also add a whatsnew for v1.0.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.
lgtm - @simonjayhawkins care to take a look as well?
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.
@ailurus1991 Thanks for the PR. a few changes for the test. if possible the test should be a similar as possible to test_filepath_or_buffer_arg
in test_formats.py
.
if you can adapt that test instead, even better!
@WillAyd if |
#28766 (comment) "We should raise when encoding is specified and it's buf isn't a string path." |
maybe it should. Since this is effectively an enhancement then it may be worth getting it right off the bat. |
…ged, and replace the ensure_clean and DF with pytest built-in tmp_path and float_frame fixture
@ailurus1991 looks ok, can you add a whtasnew note in other enhancements section |
Co-Authored-By: Simon Hawkins <simonjayhawkins@gmail.com>
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.
@ailurus1991 lgtm @jreback
@jreback Hi Jeff, I added a simple whatsnew in doc, could you review it? thx |
doc/source/whatsnew/v1.0.0.rst
Outdated
@@ -312,6 +312,7 @@ I/O | |||
- Bug in :meth:`DataFrame.to_json` where a datetime column label would not be written out in ISO format with ``orient="table"`` (:issue:`28130`) | |||
- Bug in :func:`DataFrame.to_parquet` where writing to GCS would fail with `engine='fastparquet'` if the file did not already exist (:issue:`28326`) | |||
- Bug in :meth:`DataFrame.to_html` where the length of the ``formatters`` argument was not verified (:issue:`28469`) | |||
- Added ``encoding`` argument to :func:`DataFrame.to_html` for non-ascii text (:issue:`28663`) |
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.
see #28692 (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.
move to other enhancements section
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.
@ailurus1991 Thanks for moving the whatsnew @jreback
thanks |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
added encoding argument to DataFrame.to_html()