Switch from click to argparse
#623
Conversation
|
Looks like all the tests are passing: 
|
There was a problem hiding this comment.
Pull request overview
This pull request migrates the CLI implementation from click to native Python argparse. The change affects the core CLI module, test infrastructure, documentation configuration, and dependencies.
Key changes:
- Replaced click-based CLI with argparse implementation using a custom
OptionDefclass for option definitions - Introduced a
COMMANDSregistry dictionary mapping command names to their handlers and options - Updated test fixtures to work with argparse by implementing a custom
CLIResultclass and runner function - Updated documentation to use
sphinx-argparseinstead ofsphinx-click
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| jupyter_releaser/cli.py | Complete rewrite of CLI using argparse; introduced OptionDef class, command registry, and new option handling logic |
| tests/conftest.py | Updated runner fixture to call cli.main() directly and handle output capture manually; added CLIResult class |
| tests/util.py | Changed reference from cli.main.commands to cli.COMMANDS to match new command registry |
| pyproject.toml | Removed click dependency, added sphinx-argparse to docs dependencies |
| docs/source/conf.py | Replaced sphinx-click extension with sphinxarg.ext; added myst_fence_as_directive config |
| docs/source/reference/releaser_cli.md | Updated to use argparse directive instead of click directive |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| envvar="RH_RESOLVE_BACKPORTS", | ||
| default=True, | ||
| help="Resolve backport PRs to their originals", | ||
| help_text="Resolve backport PRs to their originals", |
There was a problem hiding this comment.
The resolve-backports option has default=True but is not marked as a flag with is_flag=True. This means it will be treated as a regular string option that expects a value. However, when get_option_value handles it, a non-flag option with a truthy default will likely not be parsed correctly from environment variables or config.
For boolean options that default to True, argparse needs special handling. Consider either:
- Making this a flag and inverting the logic to
--no-resolve-backports(default True) - Or explicitly handling boolean environment variables and config values for non-flag boolean options
| help_text="Resolve backport PRs to their originals", | |
| is_flag=True, | |
| help_text="Resolve backport PRs to their originals (use --no-resolve-backports to disable)", |
| # Check environment variable first | ||
| if opt.envvar and os.environ.get(opt.envvar): | ||
| env_value = os.environ[opt.envvar] | ||
| display_value = "***" if "token" in name.lower() else env_value | ||
| util.log(f"Using env value for {name}: '{display_value}'") | ||
| if opt.is_flag: | ||
| return env_value.lower() in ("true", "1", "yes") | ||
| if opt.multiple: | ||
| return [env_value] # Environment variable is a single value for multiple | ||
| return env_value |
There was a problem hiding this comment.
The handling of environment variables for boolean options that aren't flags may cause issues. When opt.is_flag is False but the option represents a boolean (like resolve-backports), the environment variable value is returned as a string rather than being converted to a boolean. This could lead to unexpected behavior when the value is used in function calls expecting a boolean.
Consider adding logic to detect boolean-typed options and convert string environment variable values appropriately (e.g., checking if default is a bool and converting accordingly).
jupyter_releaser/cli.py
Outdated
| nargs: t.Optional[str] = None, | ||
| ): | ||
| """Initialize an OptionDef instance.""" | ||
| self.name = name | ||
| self.envvar = envvar | ||
| self.default = default | ||
| self.help_text = help_text | ||
| self.is_flag = is_flag | ||
| self.multiple = multiple | ||
| self.nargs = nargs |
There was a problem hiding this comment.
The nargs parameter is defined in the OptionDef.__init__ method and stored as an instance attribute, but it's never actually used in add_option_to_parser(). If this parameter is intended to be used for options (not just positional arguments), it should be included in the kwargs passed to parser.add_argument(). If it's only meant for positional arguments (which are handled separately), consider removing it from OptionDef to avoid confusion.
1 participant
Fixes #611