Skip to content

Fix CLI flags and scaffold assets from example notebook - #5

Open
jmfranck wants to merge 3 commits into
develfrom
codex/integrate-command_registry-and-clean-up-files
Open

Fix CLI flags and scaffold assets from example notebook#5
jmfranck wants to merge 3 commits into
develfrom
codex/integrate-command_registry-and-clean-up-files

Conversation

@jmfranck

Copy link
Copy Markdown
Owner

Summary

  • include the bundled example notebook in package data and generate short-form flags for single-letter CLI options
  • refresh the notebook scaffold to set project paths, add fallback template assets, and create missing sample render targets
  • update the notebook tests to use the new scaffold entry point

Testing

  • pytest tests/notebook/test_fast_build.py -q

Codex Task

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +759 to +763
flags = argument["flags"]
kwargs = dict(argument["kwargs"])
if "dest" in argument and argument["dest"]:
kwargs["dest"] = argument["dest"]
subparser.add_argument(*flags, **kwargs)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Skip setting dest for positional CLI arguments

The new parser builder now unconditionally injects dest for every registered argument before calling add_argument. Argparse raises ValueError: dest supplied twice for positional argument when dest is provided for positionals, so any subcommand that expects positional parameters (e.g., the many handlers that take an arguments list) will fail while constructing the parser and the pydifft CLI cannot start. Drop the dest override for non-option arguments to allow the parser to build.

Useful? React with 👍 / 👎.

Comment on lines +457 to +459
source_root = example_notebook_root()
if not source_root.exists():
raise RuntimeError("example_notebook directory is missing")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Ship example_notebook assets required by qmdinit

qmdinit now hard-requires the example_notebook directory and immediately raises a RuntimeError if it is missing, but the packaging config no longer installs any notebook templates (pyproject.toml lists only pydifftools* packages and no data files), so installed wheels will not contain example_notebook. Running pydifft qmdinit in an installed environment will therefore fail before scaffolding anything. Include the example assets in the package or provide a packaged fallback to keep qmdinit usable.

Useful? React with 👍 / 👎.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@jmfranck