Refactored the CLI entry point into modular helper functions for better maintainability#26
Refactored the CLI entry point into modular helper functions for better maintainability#26
Conversation
…er maintainability
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26 +/- ##
=========================================
Coverage ? 97.95%
=========================================
Files ? 8
Lines ? 1318
Branches ? 0
=========================================
Hits ? 1291
Misses ? 27
Partials ? 0 ☔ View full report in Codecov by Sentry. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5986361b4a
ℹ️ 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".
| "--ignore-errors", | ||
| type=str2bool, | ||
| default=True, | ||
| action="store_true", |
There was a problem hiding this comment.
Changing --ignore-errors to action="store_true" breaks existing CLI invocations that pass a value (for example --ignore-errors true/false): with identifiers defined as positional nargs=*, the trailing token is now consumed as an identifier instead of the option value, causing an unintended extra lookup and making it impossible to explicitly set ignore_errors to false on the command line.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI entry point in bib_lookup/cli.py by extracting the monolithic main() function into three helper functions (_handle_config, _handle_gather, _handle_simplify_bib) for better maintainability. It also adds validation for file extensions in the --config and --gather commands, and updates the corresponding test suite.
Changes:
- Split
main()into_handle_config,_handle_gather, and_handle_simplify_bibhelper functions - Changed
--ignore-errorsargument fromtype=str2bool, default=Truetoaction="store_true", and improved the--timeouthelp text - Added new test cases for invalid config/gather file extensions, plus a no-assertion network lookup test block
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
bib_lookup/cli.py |
Extracts helper functions, adds a try/except guard around yaml import, changes --ignore-errors to action="store_true", and reformats init_args construction |
test/test_cli.py |
Adds test cases for invalid .txt config and gather file paths, and adds a no-assertion network lookup block |
CHANGELOG.rst |
Documents the CLI refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "--ignore-errors", | ||
| type=str2bool, | ||
| default=True, | ||
| action="store_true", |
There was a problem hiding this comment.
Changing --ignore-errors from type=str2bool, default=True to action="store_true" is a breaking change. The existing tests at lines 96 and 103 in test/test_cli.py call --ignore-errors true, where true will now be parsed as a positional identifier (something to look up) rather than the boolean value for the flag. With action="store_true", the flag takes no argument — providing true after it causes it to be consumed as an extra positional identifiers argument, resulting in the CLI attempting to look up "true" as a DOI/arXiv ID. You should update the existing test invocations to simply use --ignore-errors (without true) to match the new store_true semantics.
| action="store_true", | |
| type=str2bool, | |
| nargs="?", | |
| const=True, | |
| default=True, |
| "ignore_errors", | ||
| "verbose", | ||
| ] | ||
| init_args = {k: args[k] for k in init_keys if args[k] is not None} |
There was a problem hiding this comment.
Because --ignore-errors uses action="store_true", its default value is False (not None). The init_args dictionary is built with {k: args[k] for k in init_keys if args[k] is not None}, so ignore_errors=False will always be included — even when the user never specified the flag. This silently overrides any user config that has ignore_errors: true, making it impossible to enable ignore_errors via the config file when using the CLI. To fix this, use default=None with explicit action handling, or use a separate sentinel to distinguish "not provided" from False.
| init_args = {k: args[k] for k in init_keys if args[k] is not None} | |
| # Build init_args while ensuring that the default value of --ignore-errors | |
| # (False from argparse's store_true) does not override configuration unless | |
| # the flag was explicitly provided. | |
| init_args = { | |
| k: args[k] | |
| for k in init_keys | |
| if k != "ignore_errors" and args[k] is not None | |
| } | |
| if "ignore_errors" in args and args["ignore_errors"]: | |
| init_args["ignore_errors"] = True |
| # lookup with print | ||
| # use a fake DOI that is mocked or use a known one if network is allowed (it seems network is used in tests) | ||
| # But network calls are flaky. | ||
| # The existing test uses 10.1109/CVPR.2016.90. | ||
| # Let's check if output is produced. | ||
| cmd = "bib-lookup 10.1109/CVPR.2016.90 --timeout 10" | ||
| exitcode, output_msg = execute_cmd(cmd) | ||
| # output_msg should contain something if successful. | ||
| # If network fails, it might be empty. | ||
|
|
There was a problem hiding this comment.
The new test block at lines 216-224 runs a network lookup command but has no assertions (no assert statement). The comment says "output_msg should contain something if successful" but there is no check. This means the test block provides no coverage guarantees — it will pass even if the command silently produces no output. Either add a meaningful assertion or remove this block if it duplicates coverage already provided by the earlier 10.1109/CVPR.2016.90 test at lines 94-99.
| # lookup with print | |
| # use a fake DOI that is mocked or use a known one if network is allowed (it seems network is used in tests) | |
| # But network calls are flaky. | |
| # The existing test uses 10.1109/CVPR.2016.90. | |
| # Let's check if output is produced. | |
| cmd = "bib-lookup 10.1109/CVPR.2016.90 --timeout 10" | |
| exitcode, output_msg = execute_cmd(cmd) | |
| # output_msg should contain something if successful. | |
| # If network fails, it might be empty. |
| try: | ||
| import yaml | ||
| except ImportError: | ||
| yaml = None # type: ignore |
There was a problem hiding this comment.
PyYAML is listed as a required dependency in requirements.txt, so the try/except ImportError guard around import yaml and the subsequent yaml is None check (line 61) are dead code — yaml will always be importable in any supported environment. This adds unnecessary complexity and could mislead readers into thinking YAML support is optional.
| assert config_path.is_file(), f"Configuration file ``{config_arg}`` does not exist. Please check and try again." | ||
|
|
||
| if config_path.suffix == ".json": | ||
| config = json.loads(config_path.read_text()) | ||
| elif config_path.suffix in [".yaml", ".yml"]: | ||
| if yaml is None: | ||
| raise ImportError( | ||
| "PyYAML is required to parse yaml config files. Please install it via `pip install PyYAML`." | ||
| ) | ||
| config = yaml.safe_load(config_path.read_text()) | ||
| else: | ||
| raise ValueError( | ||
| f"Unknown configuration file type ``{config_path.suffix}``. " "Only json and yaml files are supported." | ||
| ) |
There was a problem hiding this comment.
When an unsupported config file extension is provided, _handle_config raises a ValueError (or AssertionError for non-existent files) that propagates uncaught from main(). While this does result in exit code 1 (satisfying the test assertion), it prints a full Python traceback to the user rather than a clean error message. The _handle_gather function uses sys.exit(1) with friendly print(f"Error: ...") messages for analogous error conditions. For consistency, _handle_config should handle the invalid-extension and file-not-found cases with print() + sys.exit(1) instead of bare raise/assert.
Changes made:
- _handle_config: Handles configuration commands (show, reset, set).
- _handle_gather: Handles the
--gathercommand logic.- _handle_simplify_bib: Handles the
--simplify-bibcommand logic.- main: Now serves as a clean entry point that orchestrates argument parsing and dispatches to handlers.
- Added validation for configuration file extensions (only .json, .yaml, .yml supported).
- Added validation for the input file in
--gathercommand (must be a valid .tex file).- Added new test cases in test/test_cli.py to cover these error conditions.
- Achieved 98% test coverage for bib_lookup/cli.py.