ENH: multicol naive implementation. part2 - #43382
Conversation
# Conflicts: # pandas/io/formats/style.py # pandas/tests/io/formats/style/test_to_latex.py
|
@ivanovmg you fancy reviewing this one too? |
ivanovmg
left a comment
ivanovmg
left a comment
There was a problem hiding this comment.
Please see my comments below.
| ) | ||
| def test_multicol_naive(df, multicol_align, siunitx, exp): | ||
| ridx = MultiIndex.from_tuples([("A", "a"), ("A", "b"), ("A", "c")]) | ||
| df = df.astype({"A": int}) |
There was a problem hiding this comment.
Is this conversion necessary?
It seems that A column is already of int type.
There was a problem hiding this comment.
correct seemed to be legacy code (from copy/paste)
| colspan = attrs[attrs.find('colspan="') + 9 :] # len('colspan="') = 9 | ||
| colspan = int(colspan[: colspan.find('"')]) | ||
| if "naive-l" == multicol_align: | ||
| out = f"{{{display_val}}}" if wrap else f"{display_val}" |
There was a problem hiding this comment.
Is this wrap related to siunitx?
There was a problem hiding this comment.
yes. the only time wrap is needed is when siunitx is True, but it only applies to columns headers, and not row headers, due to siunitx package establishing display properties for columns
| """ | ||
| ) | ||
| s = df.style.format(precision=2) | ||
| assert expected == s.to_latex(multicol_align=multicol_align, siunitx=siunitx) |
There was a problem hiding this comment.
Would you consider extracting result and then assert result == expected?
Also, it is better to avoid one-letter variables (s in this case) to make debugging easier.
| expected = dedent( | ||
| f"""\ | ||
| \\begin{{tabular}}{{l{"SS" if siunitx else "rr"}l}} | ||
| {exp} \\\\ |
There was a problem hiding this comment.
Does this exp actually refer to a header?
Will it be better if you call it header?
| ridx = MultiIndex.from_tuples([("A", "a"), ("A", "b"), ("A", "c")]) | ||
| df = df.astype({"A": int}) | ||
| df.columns = ridx | ||
| level1 = " & a & b & c" if not siunitx else "{} & {a} & {b} & {c}" |
There was a problem hiding this comment.
I am not a fan of having logic inside tests.
I would suggest to move level1 to pytest.mark.parametrize alongside siunitx.
But it may not fit nicely in one line with the other parameters, so may be leave it as it is.
| level1 = " & a & b & c" if not siunitx else "{} & {a} & {b} & {c}" | ||
| expected = dedent( | ||
| f"""\ | ||
| \\begin{{tabular}}{{l{"SS" if siunitx else "rr"}l}} |
|
@ivanovmg changes made. I disagree with your philosophy on logic in tests. Personally I prefer parametrizations to focus as much as possible on as few parameters, and be an indicator to the purpose of the test. |
3 participants
follows #43369