fix(cli): replace union-with-None hints in CLI modules#272
fix(cli): replace union-with-None hints in CLI modules#272EnrikoMM wants to merge 1 commit intoteng-lin:mainfrom
Conversation
Use Optional/Tuple/Union typing forms in CLI helper modules to avoid runtime issues on older Python interpreters when evaluating annotations. Co-Authored-By: Oz <oz-agent@warp.dev>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughType annotations across CLI modules were systematically updated from Python 3.10-style union syntax ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates type annotations across several CLI modules, replacing the Python 3.10+ union operator (|) with Optional and Union from the typing module. The review feedback suggests maintaining consistency with PEP 585 by using built-in lowercase generics like tuple instead of importing Tuple from the typing module.
| from functools import partial | ||
| from pathlib import Path | ||
| from typing import Any, TypedDict | ||
| from typing import Any, Optional, Tuple, TypedDict |
There was a problem hiding this comment.
Since the codebase uses lowercase generics like list, dict, and tuple elsewhere (e.g., dict[str, Any] on line 181), importing Tuple from typing is unnecessary for Python 3.9+ compatibility. It's better to stick to the lowercase tuple for consistency.
| from typing import Any, Optional, Tuple, TypedDict | |
| from typing import Any, Optional, TypedDict |
References
- PEP 585: Support for the generic alias type in standard collections allows using lowercase types like 'tuple' instead of 'typing.Tuple' for type hints in Python 3.9+. (link)
|
|
||
| # Helper for file conflict resolution | ||
| def _resolve_conflict(path: Path) -> tuple[Path | None, dict | None]: | ||
| def _resolve_conflict(path: Path) -> Tuple[Optional[Path], Optional[dict]]: |
There was a problem hiding this comment.
For consistency with the rest of the codebase and other CLI modules in this PR (which use lowercase generics like list[str] and tuple[str, ...]), please use the lowercase tuple here instead of Tuple.
| def _resolve_conflict(path: Path) -> Tuple[Optional[Path], Optional[dict]]: | |
| def _resolve_conflict(path: Path) -> tuple[Optional[Path], Optional[dict]]: |
References
- PEP 585: Support for the generic alias type in standard collections allows using lowercase types like 'tuple' instead of 'typing.Tuple' for type hints in Python 3.9+. (link)
Summary
X | Noneannotations in CLI modules withOptional[X]Tuple[...]/Union[...]forms in the same pathsFiles
src/notebooklm/cli/helpers.pysrc/notebooklm/cli/download.pysrc/notebooklm/cli/download_helpers.pysrc/notebooklm/cli/skill.pysrc/notebooklm/cli/generate.pyCo-Authored-By: Oz oz-agent@warp.dev
Summary by CodeRabbit