Inclusion of new NAMED_COLORS for MS Excel #18392
Conversation
|
@jfrfonseca : Thanks for this PR! Quick question: do you know if we test that colors array in our Excel tests? If not, this might be a good time to write one to make sure we got those colors covered. |
Codecov Report
@@ Coverage Diff @@
## master #18392 +/- ##
==========================================
+ Coverage 91.57% 91.57% +<.01%
==========================================
Files 150 150
Lines 48702 48702
==========================================
+ Hits 44597 44599 +2
+ Misses 4105 4103 -2
Continue to review full report at Codecov.
|
|
@gfyoung can you push a test for this |
gfyoung
force-pushed
the
master
branch
3 times, most recently
from
3b9dd73 to
9f07018
Compare
|
@jreback : Tests have been added, and all is green. PTAL. |
|
|
||
| expected["fill"]["fgColor"] = output_color | ||
|
|
||
| if input_color in (None, "not-a-color"): |
There was a problem hiding this comment.
can you separate this into 2 tests rather than do the if (IOW good colors) and bad colors (which show a warning?)
included 'brown' (#A52A2A), 'pink' ('FFC0CB') and 'grey', US-standard spelling of the word 'gray'
|
@jreback : Change made, and all is green. PTAL. |
| } | ||
|
|
||
| with catch_warnings(record=True): | ||
| convert = CSSToExcelConverter() |
There was a problem hiding this comment.
so these don't actually raise? rather just show a warning? ok I guess
| } | ||
|
|
||
| with catch_warnings(record=True): | ||
| convert = CSSToExcelConverter() |
There was a problem hiding this comment.
so these don't actually raise? rather just show a warning? ok I guess
There was a problem hiding this comment.
so we aren't actually testing anything then?
There was a problem hiding this comment.
note - I don't think they should raise as that puts a burden back on us. is it possible to check the warnings message? (note don't spend too much time on this, if its easy do it).
There was a problem hiding this comment.
@jreback : When I call convert with an invalid color in my test case, it issues CSSWarning in 5 - 6 different places because of the invalid color (at least when I introspect with tm.assert_produces_warning).
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff