feat: Add pagination buttons (prev/next) to anywidget mode for DataFrames #1841
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
product-auto-label
bot
added
size: l
shuoweil
force-pushed
the
shuowei-anywidget-button
branch
from
080c4cb to
8125614
Compare
shuoweil
force-pushed
the
shuowei-anywidget-button
branch
from
8125614 to
0f871a9
Compare
…pis/python-bigquery-dataframes into shuowei-anywidget-button
…pis/python-bigquery-dataframes into shuowei-anywidget-button
There was a problem hiding this comment.
So cool! Thanks for adding.
Could you also add a formatter / linter to our pre-commit (https://github.com/googleapis/python-bigquery-dataframes/blob/main/.pre-commit-config.yaml)
https://github.com/biomejs/pre-commit?tab=readme-ov-file#using-biomes-pre-commit-hooks seems a good option.
repos:
- repo: https://github.com/biomejs/pre-commit
rev: v2.0.2 # Use the sha / tag you want to point at
hooks:
- id: biome-check
It's the first listed under "web" at https://pre-commit.com/hooks.html#featured-hooks
There was a problem hiding this comment.
Once you add it, you can run the pre-commit manually:
pre-commit run --files bigframes/display/table_widget.js
|
|
||
|
|
||
| def test_repr_anywidget_initial_state( | ||
| penguins_df_default_index: bf.dataframe.DataFrame, | ||
| ): | ||
| pytest.importorskip("anywidget") |
There was a problem hiding this comment.
Nit: Since all tests in this file require anywidget, let's move the importorskip to the module level.
I think this should work.
| def test_repr_anywidget_initial_state( | |
| penguins_df_default_index: bf.dataframe.DataFrame, | |
| ): | |
| pytest.importorskip("anywidget") | |
| pytest.importorskip("anywidget") | |
| def test_repr_anywidget_initial_state( | |
| penguins_df_default_index: bf.dataframe.DataFrame, | |
| ): |
Same for the other functions in this file.
|
|
||
| # Simulate next page click | ||
| widget.page = 1 | ||
| assert widget.page == 1 |
There was a problem hiding this comment.
How do we tell if the data displayed has changed? Can we check the table HTML that it doesn't contain the first page of rows and that it does contain the second page?
Rather than the penguin data, it might be better to define our own DataFrame in this test and set a really small page size so we can make sure we visualize the correct rows. See: https://testing.googleblog.com/2018/02/testing-on-toilet-cleanly-create-test.html
tests should never rely on default values that are specified by a helper method since that forces readers to read the helper method’s implementation details in order to understand the test.
| # Test going beyond last page | ||
| total_pages = math.ceil(widget.row_count / widget.page_size) | ||
| widget.page = total_pages + 1 | ||
| # Should be clamped to last valid page |
There was a problem hiding this comment.
Missing an assertion here?
| # Should stay at 0 (handled by frontend) | ||
|
|
||
| # Test going beyond last page | ||
| total_pages = math.ceil(widget.row_count / widget.page_size) |
There was a problem hiding this comment.
This is a bit too much math in a test.
https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html
Like my comment above, I recommend defining a DataFrame inside the test so that we can infer the number of pages just by looking at it rather than duplicating logic.
|
|
||
| # Test going below page 0 | ||
| widget.page = -1 | ||
| # Should stay at 0 (handled by frontend) |
There was a problem hiding this comment.
Can we do some assertion here? Maybe check that the table HTML returns the first page?
In general, we shouldn't trust the frontend to do the only validation.
| widget = TableWidget(penguins_df_default_index) | ||
|
|
||
| assert widget.page_size == 5 | ||
| total_pages = math.ceil(widget.row_count / 5) |
There was a problem hiding this comment.
Same here. Too much math. Let's define a DataFrame inside this test.
| assert widget.row_count > 0 | ||
|
|
||
| # Calculate expected pages | ||
| total_pages = math.ceil(widget.row_count / widget.page_size) |
| assert widget.page == page | ||
|
|
||
|
|
||
| def test_repr_anywidget_pagination_buttons_functionality( |
There was a problem hiding this comment.
Question: How is this test different from test_repr_anywidget_pagination_navigation? Please make the different purpose clearer in the function name.
| @property | ||
| def _esm(self): | ||
| """Load JavaScript code from external file.""" | ||
| return resources.read_text(bigframes.display, "table_widget.js") |
There was a problem hiding this comment.
We might want to cache this so that we don't have to read the file every time.
| @property | |
| def _esm(self): | |
| """Load JavaScript code from external file.""" | |
| return resources.read_text(bigframes.display, "table_widget.js") | |
| @functools.cached_property | |
| def _esm(self): | |
| """Load JavaScript code from external file.""" | |
| return resources.read_text(bigframes.display, "table_widget.js") |
https://docs.python.org/3/library/functools.html#functools.cached_property
shuoweil
added
the
kokoro:force-run
bigframes-bot
removed
the
kokoro:force-run
Adding interactive pagination buttons controls (Previous/Next buttons) to the anywidget mode for BigFrames DataFrames, enabling users to navigate through large datasets page by page in Jupyter notebooks.
b/391599608