Truncate columns list to match tr_frame for correct dict formatters lookup #35907
Conversation
| I'm not sure that overwriting self.columns is the way to go here - is there a way to localize the fix? |
| I think the new pull request localizes it down to just the spot where the fix is needed. My original fix was based on a monkey patch I was using in my library to make it work in older versions of pandas. |
WillAyd left a comment
WillAyd left a comment There was a problem hiding this comment.
Cool I think this is a better approach. Can you add a whatsnew note for 1.2?
| ("float", lambda x: f"[2] {x}"), | ||
| ("object", lambda x: f"[3] {x}"), | ||
| ] | ||
| result = df.to_string(formatters=dict(formatters), max_cols=2) |
There was a problem hiding this comment.
Rather than this can you just parametrize the inputs?
There was a problem hiding this comment.
Check out the pytest.mark.parametrize decorator - it is used by a few other tests in this module already
There was a problem hiding this comment.
I haven't used that before, but I'm still not sure what you had in mind to parameterize this method.
There was a problem hiding this comment.
I added parameterized inputs. I'm not sure if this is quite what you were expecting since there was only one pair of inputs / outputs used in the test.
There was a problem hiding this comment.
I would argue that the original version (without parametrize) was more readable. Maybe @WillAyd suggested that you parameterize separately formatters for float, int and object?
There was a problem hiding this comment.
That wouldn't exercise the fix though. It needs all of the columns in one DataFrame so that a truncated string representation can be displayed.
There was a problem hiding this comment.
The suggestion was to parametrize the arguments to formatters; one parametrize for the dict input and one for the list
| This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
| AFAIK, I've addressed all of the issues. I'm just waiting on final approval. |
| @kesmit13 yeah can you merge master and fix conflict. |
| Regarding solving the conflicts. |
doc/source/whatsnew/v1.2.0.rst Outdated
| - :class:`TableFormatter` was using the full list of columns when computing formatters for truncated displays (:issue:`35907`) | ||
| |
There was a problem hiding this comment.
Currently there is no TableFormatter.
The generic functionality is moved to DataFrameFormatter.
| def test_to_string_with_truncated_formatters(self, formatters): | ||
| df = DataFrame( |
There was a problem hiding this comment.
Can you please fix test?
Apparently, self should be removed here.
| @kesmit13, can you please merge master and resolve conflicts in |
ivanovmg left a comment
ivanovmg left a comment There was a problem hiding this comment.
Sorry for a really late reply but can you please merge master again?
I guess that your whatsnew entry should go now to v1.3.
| can you merge master and will have a look (also note would need to be in 1.4) |
| Thanks for the PR, but it appears to have gone stale. This appears close, so if you could merge master and address the remaining comments, we'd be happy to reopen and have another look over. |
8 participants
When using a dictionary of formatters and a truncated DataFrame is being displayed, the column list used for looking up the formatter in the dict are incorrect. The columns attribute of the DataFrameFormatter needs to be truncated as well for the lookup to be correct.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff