Fix documentation and argument-parsing for thumbnails export#8301
Fix documentation and argument-parsing for thumbnails export#8301snooze92 wants to merge 2 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
the
documentation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
snooze92
force-pushed
the
thumbnail_fixes
branch
from
3432bf8 to
ccb8ea0
Compare
There was a refactor where `marimo tools thumbnails generate` was replaced with `marimo export thumbnail`. The documentation still showed outdated example commands. I also found a missing ``` which broke a lot of the page. Fixed that as well.
snooze92
force-pushed
the
thumbnail_fixes
branch
from
ccb8ea0 to
58e32dc
Compare
|
docs look great, thank you! i'll let @peter-gy chime in here regarding the split CLI arguments |
There was a problem hiding this comment.
Thank you for this & apologies for the late review!
I'm aligned with the direction you already suggested to settle on a single positional target (notebook or folder) for export thumbnail, and keep notebook args forwarding.
Before merge, I’d just tighten the implementation around the final test contract so that CI becomes green.
tests/_cli/test_cli_export.py
Outdated
| _assert_success(p) | ||
|
|
||
|
|
||
| class TestSplitPathArgs: |
There was a problem hiding this comment.
It's really useful to see how the different combinations behave. Could we convert this exploratory matrix into a smaller suite to test for the single positional target support?
There was a problem hiding this comment.
I have left some tests, although it tests Click more than Marimo. I'm not convinced that it would be worth mocking a lot out to be testing the actual Click decorators used in Marimo without "duplicating" in a test.
Specifically for the thumbnails, special logic was used to support multiple paths as arguments. Unfortunately, that logic broke when using notebook arguments. Since `--` is not passed through by click when interspersed args are allowed, and every argument is wrongly extracted as a path. Removing the logic to split paths out of the arguments drops the support for multiple paths at once, restores the support to forward notebook arguments, while retaining the ability to put export options before or after the path. It makes things most consistent with other export commands. I added two tests that do run the command and assert that it completes successfully, only when the required dependencies are present and without verifying the output. I am also leaving some simple tests that test Click more than they test actual Marimo code, but might be useful as illustration/sandbox anyways.
snooze92
force-pushed
the
thumbnail_fixes
branch
from
58e32dc to
58fc7f7
Compare
3 participants
📝 Summary
As discussed on Discord, I started by simply replacing
marimo tools thumbnails generatewithmarimo export thumbnailin the relevant documentation pages (OpenGraph previews or Thumbnails).Unfortunately, I found a more sneaky issue when trying to verify the passing of notebook arguments:
marimo export thumbnail notebook.py -- --foo 123🔍 Description of Changes
The Markdown updates are very straightforward.
The support of notebook arguments took a while to get my head around! I have left a bunch of parameterised tests to show the complex way different binary choices we have to make interact with each other...
In short, we can allow interspersed arguments or not(+i / -i below), we can split paths out of the arguments or not (+s / -s below), but we can't support all 3 features:
My initial commit uses
_SPLIT_PATH_AND_ARGSand_ALLOW_INTERSPERSED_ARGSboolean flags to highlight where this is configured; as well as some illustrating tests.I would recommend that we simplify by leaving interspersed arguments enabled, but removing the logic to split path and args. We would lose the ability to have multiple paths, but get the simplest solution possible, also the most consistent with other export commands.
📋 Checklist