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: tbl_format_setup() gains a setup argument that supports printing the header before the data for the body is available, e.g., for remote backends such as databases #686

Merged
merged 6 commits into from
Dec 14, 2024

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 22, 2024

This supports printing the header before the body or footer is ready. Adding the setup argument to tbl_format_setup() was the easiest way I found to achieve this. The two commits are self-contained.

@krlmlr krlmlr changed the title f duckplyr Live printing Nov 22, 2024
@krlmlr krlmlr enabled auto-merge (squash) November 22, 2024 18:18
@krlmlr krlmlr changed the title Live printing feat: Print header first Nov 22, 2024
@krlmlr krlmlr disabled auto-merge November 22, 2024 18:20
@krlmlr
Copy link
Member Author

krlmlr commented Nov 23, 2024

This is likely to affect dbplyr by no longer showing the number of rows in the header if it's less than 21. I'd trade that for showing the header first. And I wonder if we should temporarily show the structure of the data too, and erase it (with \r?) when the body is ready.

Copy link
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

This feels quite "spooky" to me

In particular I found relying on "did setup get evaluated or not" a bit hard to understand and document.

Is it possible that new_tbl_format_setup() could gain a boolean argument that identifies whether or not this is a "partial" setup?

Like:

  • If we see is.null(setup) then we do partial setup and return with partial = TRUE.
  • That causes tbl_format_setup() to get called again, but with !is.null(setup) this time (i.e. it gets the results of the original partial setup), so we do the full setup and return with partial = FALSE

width = width,
...,
setup = {
setup_used <- TRUE
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a note in here about the fact that "If setup is evaluated by the tbl_format_setup() method, then that's our signal to recall tbl_format_setup() a second time"

header <- tbl_format_header(x, setup)
body <- tbl_format_body(x, setup)
footer <- tbl_format_footer(x, setup)
header <- transform(tbl_format_header(x, setup))
Copy link
Member

Choose a reason for hiding this comment

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

What does transform() do?

Copy link
Member Author

Choose a reason for hiding this comment

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

transform is writeLines when printing and identity when formatting. This is how we achieve incremental printing.

Adding a comment to that effect to the code.

@@ -41,6 +41,14 @@
#' This argument is mandatory for all implementations of this method.
#' @param ...
#' Extra arguments to [print.tbl()] or [format.tbl()].
#' @param setup
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth a news bullet

@krlmlr
Copy link
Member Author

krlmlr commented Nov 26, 2024

Thanks. I see how it's spooky, I couldn't think of a better way.

We want to be compatible with existing implementations of tbl_format_setup() who might not know or care about the new flow. We effectively implement a "pre-setup" (for just the header) and a "setup for real" (for the body and footer), the fallback being just one setup call. Should we do a new generic instead, or perhaps two new generics? What would they look like, what would the migration path look like?

@krlmlr krlmlr changed the title feat: Print header first feat: tbl_format_setup() gains a setup argument that supports printing the header before the data for the body is available, e.g., for remote backends such as databases Dec 7, 2024
@krlmlr krlmlr merged commit 73c00f6 into main Dec 14, 2024
5 checks passed
@krlmlr krlmlr deleted the f-duckplyr branch December 14, 2024 15:11
@krlmlr
Copy link
Member Author

krlmlr commented Dec 14, 2024

Going with this implementation for now, we can revisit as needed.

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.

2 participants