Skip to content
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

Make sure we use MIME when calling repr in GroupedDataFrame printing #3213

Merged
merged 4 commits into from Nov 5, 2022

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 30, 2022

What this PR changes:

  1. better code organization (so that later changes to DataFrameRow and GroupedDataFrame printing are needed to be made in one file).
  2. key of the PR: use MIME("text/plain) when calling repr to show group label key. This is needed to avoid printing type name in some cases (which is not useful).

This resolves the issue that before this change docs building was not passing due to the changes in InlineStrings.jl printing rules.

@bkamins bkamins added this to the patch milestone Oct 30, 2022
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but can you just detail in what case printing is changed for reference?

test/show.jl Outdated

io = IOContext(IOBuffer(), :limit=>true)
show(io, groupby(df, :id))
@test String(take!(io.io)) === "GroupedDataFrame with 3 groups based on key: id\n" *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a triple-quoted string instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to write the same comment! Something like that:

"""
GroupedDataFrame with 3 groups based on key: id
First Group (1 row): id = "a"
 Row │ id       value
     │ String1  Int64
─────┼────────────────
   1 │ a            1

Last Group (1 row): id = "c"
 Row │ id       value
     │ String1  Int64
─────┼────────────────
   1 │ c            3
"""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will re-check it (this is what I did initially, but I was getting test errors constantly for some reason I did not understand)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I have managed to find problems. The issues were trailing whitespace in some cases.

@bkamins
Copy link
Member Author

bkamins commented Nov 2, 2022

What we currently have is:

julia> using DataFrames

julia> using InlineStrings

julia> df = DataFrame(id=inlinestrings(["a", "b"]), x=1:2)
2×2 DataFrame
 Row │ id       x
     │ String1  Int64
─────┼────────────────
   1 │ a            1
   2 │ b            2

julia> groupby(df, :id)
GroupedDataFrame with 2 groups based on key: id
First Group (1 row): id = String1("a")
 Row │ id       x
     │ String1  Int64
─────┼────────────────
   1 │ a            1
⋮
Last Group (1 row): id = String1("b")
 Row │ id       x
     │ String1  Int64
─────┼────────────────
   1 │ b            2

As you can see "a" and "b" are printed with String1 wrapper due to recent changes in InlineStrings.jl.

While we have:

repr("text/plain", x) is typically a "pretty-printed" version of x designed for human consumption.

which is, in general closer to what we want in this case.

After this PR the String1 wrapper is not printed.

@bkamins bkamins mentioned this pull request Nov 2, 2022
@bkamins bkamins merged commit a1dc899 into main Nov 5, 2022
@bkamins bkamins deleted the bk/fix_docs_print branch November 5, 2022 10:05
@bkamins
Copy link
Member Author

bkamins commented Nov 5, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants