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

feat(ux): add Table and Column.preview() #7408

Closed
wants to merge 1 commit into from

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Oct 20, 2023

fixes #7172

I wasn't sure how testing this should work so I didn't add any tests. I would really appreciate it if someone more familiar could pick this up and finish it off, I think I've gotten a lot of it out of the way.

Hopefully I've implemented this is such a way that it is testable.

Things I'm not sure about:

  1. adding **fmt_kwargs all over pretty.py. I didn't really want all the boilerplate that would be required if I was explicit with each kwarg. Possibly could pass around a container object such as options.Repr instead.
  2. I encapsulated some logic into pretty.pretty_repr() because I thought that was going to be necessary for the functional change. It ended up not being needed, but I still think it is a good refactor to tie up some disparate ends. I can revert this though if you want.

@NickCrews
Copy link
Contributor Author

NickCrews commented Oct 20, 2023

doctests seems to be failing, I think because

  1. returning a rich Table and having the repl call repr() on it
  2. printing that rich Table directly to the console

aren't quite equivalent. But I don't really see what should be done here, should 2 be what happens in both .preview() and interactive_rich_console()? I'm not that famliar with the rich API/stack.

@NickCrews
Copy link
Contributor Author

I would love to get this fixed and merged. Does this look like the generally right idea? Can you point me in the right direction on my rich API question above (and I can fix it up myself)?

@NickCrews
Copy link
Contributor Author

Hey @cpcloud if I figure out the issues and fix them will this get merged? I don't want to do that effort though if this isn't the right direction at all. Thanks!

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Meant to give this a review!

I think this is a useful method for folks that want to customize the way the output looks.

self,
*,
max_rows: int | None = None,
show_types: bool | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this out for now. I'm not even sure it's a useful option to have in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by "this" do you just mean the "show_types" arg? yeah I sort of agree I feel like it should always be True

Copy link
Member

Choose a reason for hiding this comment

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

Yep, was asking about show_types.

max_length: int | None = None,
max_string: int | None = None,
max_depth: int | None = None,
console_width: int | float | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Are there cases when this is a useful option to set when using ibis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So one of the motivators for this PR was better UX when using the VSCode debugger. Currently this PR doesn't quite work yet:
image
Ideally this would show the table nicely. But I noticed when I did the workaround of converting to pandas, that the console width seems to be small and I get ugly wrapping:
image

I was guessing that this console_width param was needed so the output would be pretty. But perhaps I was getting ahead of myself. we leave it out for now, and cross that bridge when we come to it?

@NickCrews
Copy link
Contributor Author

@cpcloud a larger API question is: Should .preview() return a rich.Table for the caller/framework to render, or should it print to stdout directly? (ie the same difference as ibis.to_sql() vs ibis.show_sql())

I might argue for a kwarg like def preview(..., file: io.StringIO | None = None), so the default is to just print to stdout.
This is because I imagine this method is mostly going to get used in notebooks or debuggers. For example, in the vscode debugger, if I want to see an ibis table, I have to print() it, otherwise I get the raw output with ANSI codes:

image image

I would like it to work where I can just call t.preview() and it works.

@cpcloud cpcloud deleted the branch ibis-project:master January 4, 2024 10:42
@cpcloud cpcloud closed this Jan 4, 2024
@cpcloud
Copy link
Member

cpcloud commented Jan 4, 2024

@NickCrews Would you mind reopening this PR against main?

NickCrews added a commit that referenced this pull request Apr 12, 2024
redo of #7408 targeting main instead of master

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

Successfully merging this pull request may close these issues.

feat: ux: inline way to adjust interactive config
2 participants