Fix documentation and argument-parsing for thumbnails export#8301
Open
snooze92 wants to merge 2 commits intomarimo-team:mainfrom
Open
Fix documentation and argument-parsing for thumbnails export#8301snooze92 wants to merge 2 commits intomarimo-team:mainfrom
snooze92 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 ✍️ ✅ |
Author
|
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.
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. In this commit, I knowingly include broken tests. I do not intend to merge this as-is, but I will need the Marimo team input to decide how we want to fix this. Those tests are there simply to illustrate the discussion. The two `_SPLIT_PATH_AND_ARGS` and `_ALLOW_INTERSPERSED_ARGS` are also here purely for illustration. We should inline this once we decide for one approach. I also add two simple tests that should stay, to confirm that the kind of examples from the documentation do run successfully.
snooze92
force-pushed
the
thumbnail_fixes
branch
from
ccb8ea0 to
58e32dc
Compare
Contributor
|
docs look great, thank you! i'll let @peter-gy chime in here regarding the split CLI arguments |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
📝 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