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

REF: type change escape in Styler.format to str to allow "html" and "latex" #41619

Merged
merged 16 commits into from
May 27, 2021

Conversation

attack68
Copy link
Contributor

@attack68 attack68 commented May 22, 2021

Change is made ahead of 1.3.0 to pre-empt the addition of escape_latex and to avoid name ambiguity.

This is not user facing since the escape kwarg was added for 1.3.0 only recently.

Alternative

If instead of having separate escape_html and escape_latex kwargs we could have escape={None, 'html', 'latex'} since it is likely they will be used exclusively??

@simonjayhawkins simonjayhawkins added Refactor Internal refactoring of code Styler conditional formatting using DataFrame.style labels May 24, 2021
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can u move this to the to_html or to_latex method instead?

@attack68
Copy link
Contributor Author

can u move this to the to_html or to_latex method instead?

no, because it's mixed in as part of the formatting routine, and the idea is that DataFrames should be formatted before calling to_html or to_latex, i.e.

df.style.format(...).to_xxx()

if escape is added to the render method then format needs to be called within the render method overwriting the previous chained application and negating it all and restricting its flexibility.

what about expanding the keyword inputs to:

escape=None,
escape='html'
escape='latex'
escape=['html', 'latex']

?

@jreback
Copy link
Contributor

jreback commented May 25, 2021

yeah the last is fine i guess

but is there a way to just escape things that work in both html and latex?

@attack68
Copy link
Contributor Author

attack68 commented May 25, 2021

yeah the last is fine i guess

but is there a way to just escape things that work in both html and latex?

not currently because of operation order. the format method is ignorant of how the styler will ultimately be rendered, and it yields a one-arg-callable for each data element decoupled from render method. if it becomes dependent it introduces operation order complexity and possible performance degradation.

(and of course html and latex have different escaped chars)

will propose the former alternative and can review again.

@attack68 attack68 changed the title REF: rename kw escape escape_html in Styler.format REF: type change escape in Styler.format to str to allow "html" and "latex" May 25, 2021
@attack68 attack68 marked this pull request as draft May 26, 2021 08:15
@attack68
Copy link
Contributor Author

attack68 commented May 26, 2021

@jreback it turns out some of the escaped characters overlap so it is impossible to escape 'html' and 'latex' simultaneously.

I've edited this PR to allow escape in {None, 'html', 'latex'} and added the necessary latex escape function and tests.

@attack68 attack68 marked this pull request as ready for review May 26, 2021 08:58
pandas/io/formats/style_render.py Show resolved Hide resolved
@@ -1187,3 +1207,38 @@ def _parse_latex_options_strip(value: str | int | float, arg: str) -> str:
For example: 'red /* --wrap */ ' --> 'red'
"""
return str(value).replace(arg, "").replace("/*", "").replace("*/", "").strip()


def _escape_latex(s):
Copy link
Contributor

Choose a reason for hiding this comment

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

where is eacape_html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imported from markupsafe which is a jinja2 dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a very simple function, can write here instead of having dependency to markupsafe if preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh its fine, just not completely obvious where it is (but its already there so its fine)

@@ -1187,3 +1207,38 @@ def _parse_latex_options_strip(value: str | int | float, arg: str) -> str:
For example: 'red /* --wrap */ ' --> 'red'
"""
return str(value).replace(arg, "").replace("/*", "").replace("*/", "").strip()


def _escape_latex(s):
Copy link
Contributor

Choose a reason for hiding this comment

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

oh its fine, just not completely obvious where it is (but its already there so its fine)

@jreback jreback added this to the 1.3 milestone May 27, 2021
@jreback jreback merged commit 433860b into pandas-dev:master May 27, 2021
@attack68 attack68 deleted the html_escape branch May 27, 2021 21:23
TLouf pushed a commit to TLouf/pandas that referenced this pull request Jun 1, 2021
@attack68 attack68 mentioned this pull request Jun 7, 2021
1 task
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Internal refactoring of code Styler conditional formatting using DataFrame.style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants