Refactor debug rendering #2356
Merged
Refactor debug rendering #2356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying databuilder-docs with
|
| Latest commit: |
9761f4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b17fa1b2.databuilder.pages.dev |
| Branch Preview URL: | https://refactor-debug-rendering.databuilder.pages.dev |
The sandbox method of evaluating and rendering ehrql objects was via monkey patching various repr functions The initial debug implementation re-used this method, and manually called `repr()` from within the show() call. Now that we don't have a sandbox, all rendering can be done explicitly by the show() function, so there is no need for repr monkey patching. This refactor switches the evaluate/render steps to be done explicitly in show(), rather than going via repr. However, the repr monkey patching implementation used a closure to capture the supplied renderer and engine details in the monkeypatched function. As `show()` is a user imported static function, it cannot use a closure. Instead, we formalize the DEBUG_QUERY_ENGINE global into a new DEBUG_CONTEXT global, which is set via `activate_debug_context`, and then available to use inside `show()`. This DEBUG_CONTEXT stores the engine instance and the renderer, and knows how to render and object with those.
…e test file line numbers
bloodearnest
force-pushed
the
refactor-debug-rendering
branch
from
a5052f2 to
6936034
Compare
bloodearnest
force-pushed
the
refactor-debug-rendering
branch
from
6936034 to
dff625e
Compare
bloodearnest
force-pushed
the
refactor-debug-rendering
branch
from
dff625e to
c6f7093
Compare
bloodearnest
force-pushed
the
refactor-debug-rendering
branch
from
c6f7093 to
b6da2eb
Compare
Previously, we did the truncation post render, probably in order not to mess with the default repr, which cannot be parametrised with head/tail arguments. However, that is gone now, and we no longer render via a parameterless repr, so we refactor the truncation at render time. This has several advantages - can use the same logic for both html and ascii rendering - only done in one place - more efficient, as we do not render the truncated rows at all. This includes the following changes: - head and tail are arguments to the render() function call. The user interface in show() allows them to be `int | None`, but we force them to integer values in the implementation, with 0 meaning None, as both are falsey. This necessetated some changes to not use _render_ functions, and call to_records() directly, so we could pass the head/tail args at render time. - head and tail logic is factored out into a shared function, and handles ellipsis logic consistently. - The render function's `records` parameter was annotated as list[dict], but it was actually a generator. We assume that it is a list, and use indexing for simple head/tail splicing, and fix the call sites to list() the the generator. There's no way to do tail w/o exhausting the generator anyway, and we were rendering the full thing before, so this is no less efficnet, but is simpler to read. - fixed the debug tests to always use a json renderer for testing, for consistency.
bloodearnest
force-pushed
the
refactor-debug-rendering
branch
from
b6da2eb to
d00b3e3
Compare
rw251
reviewed
Jan 17, 2025
ehrql/renderers.py
Outdated
| return start + truncated_rows + end | ||
| def records_to_html_table(records: list[dict], head: int = 0, tail: int = 0): | ||
| rows = [] | ||
| headers = "".join([f"<th>{header}</th>" for header in records[0].keys()]) |
There was a problem hiding this comment.
Could records ever be an empty list? If so then I think this throws an error now which wouldn't have happened before. Same comment on the records_to_ascii_table method.
There was a problem hiding this comment.
Good catch, will fix. It might be better to supply the header info independently of the rows, so we can show headers for an empty table, but I am not sure how to do that, so I will go for just not breaking.
rw251
reviewed
Jan 17, 2025
| table_rows[headers : headers + head] if head is not None else [ellipsis_row] | ||
| ) | ||
| truncated_rows.extend(head_rows) | ||
| return f"{START_MARKER}<table><thead><tr>{headers}</tr></thead><tbody>{html_rows}</tbody></table>{END_MARKER}" |
There was a problem hiding this comment.
Do we still need the START_MARKER and END_MARKER? Or were they just so the previous regex would find the table?
There was a problem hiding this comment.
Yes, they are not for the regex, but for the vscode extension to know where pre-formatted html tables start and end (this bit is what I am working on now).
There was a problem hiding this comment.
Ah of course. I'd naively just searched the ehrql code space for the marker and forgot about the extension.
rw251
reviewed
Jan 17, 2025
|
|
||
| """ | ||
| ).strip() | ||
|
|
||
| show("Hello") | ||
| exec( |
rw251
approved these changes
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The sandbox method of evaluating and rendering ehrql objects was via
monkey patching various repr functions The initial debug implementation
re-used this method, and manually called
repr()from within the show()call.
Now that we don't have a sandbox, all rendering can be done explicitly
by the show() function, so there is no need for repr monkey patching.
This refactor switches the evaluate/render steps to be done explicitly
in show(), rather than going via repr.
However, the repr monkey patching implementation used a closure to
capture the supplied renderer and engine details in the monkeypatched
function. As
show()is a user imported static function, it cannot usea closure. Instead, we formalize the DEBUG_QUERY_ENGINE global into
a new DEBUG_CONTEXT global, which is set via
activate_debug_context,and then available to use inside
show(). This DEBUG_CONTEXT stores theengine instance and the renderer, and knows how to render and object
with those.
Also includes some follow refactors around truncation and tests.