Use cssdecl package for resolving CSS #16170
Conversation
| @@ -169,11 +169,13 @@ | |||
| {'alignment': {'wrap_text': True}}), | |||
| ]) | |||
| def test_css_to_excel(css, expected): | |||
| pytest.importorskip('cssdecl') | |||
There was a problem hiding this comment.
the main thing to test is what happens when cssdecl is not installed and to_excel is called
need an informative error message
There was a problem hiding this comment.
do you have an assertion for exception message checking in your testing arsenal?
ci/requirements-2.7_LOCALE.run
Outdated
| @@ -4,6 +4,7 @@ numpy=1.8.2 | |||
| xlwt=0.7.5 | |||
| openpyxl=1.6.2 | |||
| xlsxwriter=0.4.6 | |||
| cssdecl | |||
There was a problem hiding this comment.
don't add this to all of these
just 2.7 and 3.6 is enough
u can add the os x build if u want
windows prob 2 (only need to add to 1)
There was a problem hiding this comment.
okay. I meant to clarify but forgot.
|
Do get this working, does cssdecl need uploading to the pandas channel? Currently it's at https://github.com/jnothman/cssdecl |
|
@jnothman nothing to do with a channel. your package should be pip installable. |
|
Are you certain? it is definitely pip installable... and the error messages in travis are like: |
|
You should be able to add it to |
|
Thanks
…On 1 May 2017 at 23:49, Tom Augspurger ***@***.***> wrote:
You should be able to add it to ci/requirements-*.pip
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16170 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz66esBFa0ARy6z52c5zYmhB9obmCpks5r1eLbgaJpZM4NMOkW>
.
|
|
Test failure appears unrelated |
Codecov Report
@@ Coverage Diff @@
## master #16170 +/- ##
==========================================
- Coverage 90.86% 90.64% -0.22%
==========================================
Files 162 161 -1
Lines 50819 50718 -101
==========================================
- Hits 46177 45975 -202
- Misses 4642 4743 +101
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #16170 +/- ##
==========================================
- Coverage 90.38% 90.13% -0.25%
==========================================
Files 161 160 -1
Lines 50916 50807 -109
==========================================
- Hits 46021 45797 -224
- Misses 4895 5010 +115
Continue to review full report at Codecov.
|
pandas/tests/io/test_excel.py
Outdated
| with pytest.raises(ImportError) as rec: | ||
| pd.DataFrame({"A": [1, 2]}).style.to_excel("openpyxl") | ||
|
|
||
| assert 'not installed' in rec.value.msg |
There was a problem hiding this comment.
Ahh. That's how to do it with pytest. Better here than above (https://github.com/pandas-dev/pandas/pull/16170/files#diff-cd0301639e13a1419d0c4522f08f2be8R2520)?
There was a problem hiding this comment.
Except that it seemed to fall CI
There was a problem hiding this comment.
:( will push a fix soon
|
this PR needs to wait for 0.21.0 fyi. would like to have the dep filled out / more testing and docs. |
|
okay. what do you mean by "the dep filled out"?
…On 3 May 2017 9:45 am, "Jeff Reback" ***@***.***> wrote:
this PR needs to wait for 0.21.0 fyi. would like to have the dep filled
out / more testing and docs.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16170 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6wKDjowvs1IArJA27SSR6r67iDklks5r18AogaJpZM4NMOkW>
.
|
|
@jnothman what I mean is that this dep is used, tested on CI, has docs etc. Yes I know you carved it out of pandas, but it needs to be a properly supported (and versioned package). Its not a big deal to do that (and it kind of forces you to make it user friendly). We are going to start pushing to 0.21.0 soon anyhow (right after the release of 0.20.0) |
|
Yes, I thought you meant CI and docs. I can't make it used.
…On 3 May 2017 at 10:29, Jeff Reback ***@***.***> wrote:
@jnothman <https://github.com/jnothman> what I mean is that this dep is
used, tested on CI, has docs etc. Yes I know you carved it out of pandas,
but it needs to be a properly supported (and versioned package). Its not a
big deal to do that (and it kind of forces you to make it user friendly).
We are going to start pushing to 0.21.0 soon anyhow (right after the
release of 0.20.0)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16170 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6wAHywafI42LMjJNTtQUquXxlczxks5r18pbgaJpZM4NMOkW>
.
|
pandas/tests/io/test_excel.py
Outdated
|
|
||
| def assert_equal_style(cell1, cell2): | ||
| # XXX: should find a better way to check equality | ||
| assert cell1.alignment.__dict__ == cell2.alignment.__dict__ |
There was a problem hiding this comment.
you can just assert ceil1.alignment == cell2.alignment, though much better would be to add an __eq__ method to cell itself.
There was a problem hiding this comment.
Cell is in openpyxl. Are you suggesting I monkeypatch??
There was a problem hiding this comment.
And I can't recall why right now, but testing for equality of cell.alignment did not work.
There was a problem hiding this comment.
In [7]: import openpyxl
In [8]: a1 = openpyxl.Workbook().create_sheet()['A1']
In [9]: a2 = openpyxl.Workbook().create_sheet()['A2']
In [11]: a1.alignment = openpyxl.styles.Alignment(horizontal='center')
In [12]: a2.alignment
Out[12]:
<openpyxl.styles.alignment.Alignment object>
Parameters:
wrapText=None, vertical=None, readingOrder=0.0, textRotation=0, relativeIndent=0.0, horizontal=None, justifyLastLine=None, indent=0.0, shrinkToFit=None
In [13]: a2.alignment == a1.alignment
Out[13]: False
In [14]: a2.alignment = openpyxl.styles.Alignment(horizontal='center')
In [15]: a2.alignment == a1.alignment
Out[15]: False
pandas/tests/io/test_excel.py
Outdated
| @@ -2490,3 +2499,71 @@ def custom_converter(css): | |||
| n_cells += 1 | |||
|
|
|||
| assert n_cells == (10 + 1) * (3 + 1) | |||
|
|
|||
There was a problem hiding this comment.
I would split the style tests off into another test file. you can in fact make a sub-dir excel and move test_excel.py there as well (not that if you do this, you need to add a __init__.py and add the dir to setup.py.
There was a problem hiding this comment.
Not sure I'm getting you entirely, as I'm trying to work out how this relates to the boundaries between io/excel and io/formats/excel. Do you want to see this all merged?
IMO, a logical structure for the code might be:
pandas/io/excel/read.py-*Readerpandas/io/excel/write.py-ExcelFormatter,*Writerpandas/io/excel/style.py-CSSToExcelConverter
with test files to correspond.
Or perhaps pandas/io/excel/format.py should include CSSToExcelConverter and ExcelFormatter as pandas/io/formats/excel.py does currently.
However, the tests you're concerned to move here are end-to-end tests covering the Styler.to_excel interface and writers.
Please clarify your vision if you can.
There was a problem hiding this comment.
the code org and the test org ideally will mirror each other. but they can be moved independently. for now I want smaller / more well defined / does 1 thing test files. I think this is a nice way to organize things into chunks. Then can prob tackle the code reorg.
The formatting code lives in pandas/io/formats/*, and style fits right in here.
Style shouldn't be directly connected (nor depended on in any way) in the core code. It is simply output formatting.
There was a problem hiding this comment.
I think you're saying that this test belongs in pandas/tests/io/formats...?
There was a problem hiding this comment.
yes move all style things there (in test_to_excel)
|
this looks reasonable. pls rebase and ping when ready. |
| @@ -0,0 +1 @@ | |||
| cssdecl | |||
There was a problem hiding this comment.
add to ci/requirements-3.5.pip & requirements-2.7.pip as well (what you added here barely tests anything and only on windows).
There was a problem hiding this comment.
Oh. Strange. I think I must have misperformed a patch commit.
There was a problem hiding this comment.
I'd presume 3.6, 3.5_DOC and 2.7_WIN sufficient, though.
There was a problem hiding this comment.
yes that is fine. note that you might want to create a conda-forge package for this at some point (otherwise you can't include it directly in a dependecy tree as they don't support pip install directly).
ci/requirements_all.txt
Outdated
| @@ -9,6 +9,7 @@ python-dateutil | |||
| pytz | |||
| openpyxl | |||
| xlsxwriter | |||
| cssdecl | |||
There was a problem hiding this comment.
this is for developement and not necessary to have this, pls remove.
|
add a note in api-breaking in 0.21.0 |
|
Done apart from directory restructure. |
|
So is this okay for now? |
|
re conda-forge, I'm still learning the ropes a little, but have at least built a package and posted it to my own channel. |
yeah you can submit a recipe to conda-forge and then it will create a feedstock. should be pretty easy for this. not required, but nice. |
| @@ -249,6 +249,7 @@ Optional Dependencies | |||
| * `openpyxl <http://packages.python.org/openpyxl/>`__: openpyxl version 1.6.1 | |||
| or higher (but lower than 2.0.0), or version 2.2 or higher, for writing .xlsx files (xlrd >= 0.9.0) | |||
| * `XlsxWriter <https://pypi.python.org/pypi/XlsxWriter>`__: Alternative Excel writer | |||
| * `cssdecl <https://pypi.python.org/pypi/cssdecl>`__: along with one of openpyxl or XlsxWriter for exporting styled DataFrames | |||
There was a problem hiding this comment.
if you have a min version requirement we can put it here.
There was a problem hiding this comment.
I've decided I can probably keep the interface backwards compatible. There may, in the future, be min versions for certain features, e.g. gradient fills used for bar charts, but there will also be a min version of openpyxl for that (and I don't think xlsxwriter supports)
There was a problem hiding this comment.
xlsxwriter support mentioned here depends largely on #16149 being merged
| ['', '', '']], | ||
| index=df.index, columns=df.columns) | ||
|
|
||
| pytest.importorskip('jinja2') |
There was a problem hiding this comment.
does this also need cssdecl to be imported?
There was a problem hiding this comment.
are you asking if the fixture also needs cssdecl to be imported? no, since we allow for a custom style_converter.
| pytest.importorskip(engine) | ||
|
|
||
| if engine == 'openpyxl' and openpyxl_compat.is_compat(major_ver=1): | ||
| pytest.xfail('openpyxl1 does not support some openpyxl2-compatible ' |
There was a problem hiding this comment.
you can skip rather than xfail here
| @@ -0,0 +1 @@ | |||
| cssdecl | |||
There was a problem hiding this comment.
can you add to requirements-2.7 as well
doc/source/whatsnew/v0.21.0.txt
Outdated
| @@ -34,6 +34,7 @@ Other Enhancements | |||
| Backwards incompatible API changes | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| - The ``DataFrame.style.to_excel()`` styled Excel export functionality now requires an external dependency, `cssdecl <http://pypi.python.org/pypi/cssdecl>`_. | |||
There was a problem hiding this comment.
actually add a little note in io.rst (about the change as well)
There was a problem hiding this comment.
I assume you mean style.ipynb
There was a problem hiding this comment.
doesn't this add export to xlxswriter as compared to 0.20.1? if so pls add another note in Other Enhancements section. (maybe also need to update the io.rst docs?)
There was a problem hiding this comment.
sorry I see you mentioned that above. nvm. then.
|
Green, @jreback |
|
can you rebase on master. skips were not printing correctly, so now fixed. |
doc/source/whatsnew/v0.21.0.txt
Outdated
| @@ -34,6 +34,7 @@ Other Enhancements | |||
| Backwards incompatible API changes | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| - The ``DataFrame.style.to_excel()`` styled Excel export functionality now requires an external dependency, `cssdecl <http://pypi.python.org/pypi/cssdecl>`_. | |||
There was a problem hiding this comment.
just add the PR number (this number) to the end of the whatsnew. & rebase. ping on green to merge.
|
xlsxwriter support requires #16149
…On 12 May 2017 9:37 am, "Jeff Reback" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/source/whatsnew/v0.21.0.txt
<#16170 (comment)>:
> @@ -34,6 +34,7 @@ Other Enhancements
Backwards incompatible API changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+- The ``DataFrame.style.to_excel()`` styled Excel export functionality now requires an external dependency, `cssdecl <http://pypi.python.org/pypi/cssdecl>`_.
doesn't this add export to xlxswriter as compared to 0.20.1? if so pls add
*another* note in Other Enhancements section. (maybe also need to update
the io.rst docs?)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16170 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6w-2Do0NKWQs2SxeBn5E3W_xMwe_ks5r45PagaJpZM4NMOkW>
.
|
|
PR number added to what's new. Master merged. cssdecl's next release (and I'll try to package it in conda-forge too) should at least handle some CSS cleaning better and handle |
|
I've just been informed of the package weasyprint which does everything cssdecl should do, it seems. My literature review want good enough. Let me look a bit further into whether we can use that rather than reinvent the wheel. Closing for now. |
Uses dependency on an external project instead of having local code to handle the resolution of DataFrame.style CSS to interpretable atoms. This allows reuse of that code and relatively independent development of more complete CSS2.2 coverage.
closes #xxxxN/Agit diff upstream/master --name-only -- '*.py' | flake8 --diffwhatsnew entryN/A