-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
EHN: add ability to format index and col names to Styler #57880
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
Conversation
Hi thanks for this PR. A
Since a method addition is quite a significant undertaking. I would ask that you split this PR into multiple stages. From what I can see you have followed the template of previous code and that is a great start. |
9062cff
to
3cbe658
Compare
Hi @attack68, I've updated my code. It's now my attempt at the first point in your list. |
I think we should probably call this method I think we also need greater test coverage to cover lines. You should probably look to replicate the tests for the specific case :
|
Since your code also involves cases where things are hidden you should also include a test to show that, say if a multi-index has a level hidden, the format function still works correctly on the levels that are still shown. |
Co-authored-by: JHM Darbyshire <[email protected]>
Co-authored-by: JHM Darbyshire <[email protected]>
@attack68 added a bunch of new tests. don't immediately see the point of |
Are you familiar with |
pandas/io/formats/style_render.py
Outdated
|
||
Notes | ||
----- | ||
This method has a similar signature to :meth:`Styler.format_index`. Since `names` are generally label |
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.
pls fix this indentation. my fault from previous i expect
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.
fixed
Yeah. I ran coverage and see my new method is well covered. Mostly because i reuse a lot of things, as you have pointed out. So i don't really understand what your suggestion is. Can you elaborate? Or maybe most of the material is reuse so you suggest we could achieve the same coverage without so much test? I hope not. |
My point is you need to cover your code with your tests. Aim for 100%. It seems you have achieved that. Do you agree? |
Thought so too. If there is nothing major, I think i would fix up whats left and wrap this up. Then ill start to work on the next point in a new PR. In case if things go stale, this PR can still be merged as an enhancement on its own. (ofc versionadded and whatsnew have to be filled before merge). Does that sound good to you? |
Indeed, is what I would suggest. Thanks for the good work here and following the structure. Hopefully the other reviewers will recognise this as quite a tidy addition and be fairly quick approve also. I'll look to add my approval when all the bits in and tests pass. There is one additional file you have to add to: doc/source/reference/style.rst |
<thead> | ||
<tr> | ||
<th class="blank" > </th> | ||
<th class="index_name level0" >{expected_columns[0]}</th> | ||
<th id="T_test_level0_col0" class="col_heading level0 col0" colspan="2">A</th> | ||
<th id="T_test_level0_col2" class="col_heading level0 col2" colspan="2">B</th> | ||
</tr> | ||
<tr> | ||
<th class="blank" > </th> | ||
<th class="index_name level1" >{expected_columns[1]}</th> | ||
<th id="T_test_level1_col0" class="col_heading level1 col0" >a</th> | ||
<th id="T_test_level1_col1" class="col_heading level1 col1" >b</th> | ||
<th id="T_test_level1_col2" class="col_heading level1 col2" >a</th> | ||
<th id="T_test_level1_col3" class="col_heading level1 col3" >b</th> | ||
</tr> | ||
<tr> | ||
<th class="index_name level0" >{expected_index[0]}</th> | ||
<th class="index_name level1" >{expected_index[1]}</th> |
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.
Its probably sufficient to assert expected
in result
rather than equate the large bulk of text. This also future proofs the test so that if other changes are implemented which dont affect the specific format_index_names function this test wont need updating.
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.
updated
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.
Needs a whatsnew
entry.
Otherwise LGTM.
Do you know the target version? Or should we wait for an active member? |
pandas/io/formats/style_render.py
Outdated
When using a ``formatter`` string the dtypes must be compatible, otherwise a | ||
`ValueError` will be raised. |
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 put this in its own
Raises
------
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.
updated
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.
Could you add a whatsnew note in v3.0.0.rst
under Other Enhancements
?
Co-authored-by: Matthew Roeschke <[email protected]>
Co-authored-by: Matthew Roeschke <[email protected]>
Added |
Thanks @quangngd |
@quangngd are you interested to work on stage 2 of this. I think it would be good to get in so that there is no disconnect for users between these functions and the LaTeX output. To my knowledge there are currently no undocumented issues between the styler and the LaTeX output so good for me to keep an eye on. |
@attack68 |
…57880) * add new method to styler * add html test * fix type * rename to format_index_names * Update pandas/io/formats/style_render.py Co-authored-by: JHM Darbyshire <[email protected]> * Update pandas/io/formats/style_render.py Co-authored-by: JHM Darbyshire <[email protected]> * add tests * add test * more doc * doc * update code_checks * add example * update test * Update pandas/io/formats/style_render.py Co-authored-by: Matthew Roeschke <[email protected]> * Update pandas/io/formats/style_render.py Co-authored-by: Matthew Roeschke <[email protected]> * update doc --------- Co-authored-by: JHM Darbyshire <[email protected]> Co-authored-by: Matthew Roeschke <[email protected]>
Styler.format_index
#47489doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.xref #57362
Stage one. Part of an multi-stages effort: #57880 (comment)
Adding a method to Styler and ensuring it works for the default HTML cases with tests in the appropriate pages.