Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
+ Coverage 70.60% 71.67% +1.07%
==========================================
Files 47 48 +1
Lines 2885 3033 +148
Branches 641 678 +37
==========================================
+ Hits 2037 2174 +137
- Misses 738 740 +2
- Partials 110 119 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements a belay add command that adds dependencies to pyproject.toml with support for multiple URI formats. The implementation follows a "download first, then save" approach to ensure dependencies are valid before modifying the project configuration.
Key changes include:
- New CLI command
belay addfor adding dependencies with automatic package name inference - Version suffix parsing supporting both
@versionand==versionsyntax - Extended Git provider URL parsing to handle repository root URLs (e.g.,
https://github.com/user/repo)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
belay/cli/add.py |
Implements the add command with URI inference, dependency validation, and TOML configuration management |
tests/cli/test_add.py |
Comprehensive test suite for add command covering various URI formats, error cases, and CLI integration |
belay/cli/main.py |
Registers the add command with the CLI app |
belay/packagemanager/downloaders/git.py |
Adds split_version_suffix() function and inferred_package_name property for smart package naming |
belay/packagemanager/downloaders/_github.py |
Extends GitHub URL parser to handle repo root URLs without file paths |
belay/packagemanager/downloaders/_gitlab.py |
Extends GitLab URL parser to handle repo root URLs without file paths |
belay/packagemanager/downloaders/_package_json.py |
Refactored to use centralized split_version_suffix() function |
belay/helpers.py |
Adds sanitize_package_name() utility for converting raw names to valid Python identifiers |
belay/project.py |
Enhances ProjectCache with type hints and overload decorators for better type safety |
tests/packagemanager/downloaders/test_common.py |
Adds tests for new Git URL parsing features and version suffix splitting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
belay/cli/add.py
Outdated
| pass | ||
|
|
||
|
|
||
| def _get_dependencies_table(doc, group: str): |
There was a problem hiding this comment.
Missing return type annotation. While the docstring mentions "dict-like", it should specify a concrete return type annotation for better type safety.
belay/cli/add.py
Outdated
| ---------- | ||
| name_or_uri : str | ||
| Package name (if uri is provided) or source URI (if uri is omitted). | ||
| uri : str |
There was a problem hiding this comment.
The parameter type in the docstring should indicate that uri is optional (e.g., "str, optional" or "Optional[str]") to match the function signature where uri: Optional[str] = None.
| uri : str | |
| uri : str, optional |
belay/cli/add.py
Outdated
| raise ValueError(f"Package name '{package}' must be a valid Python identifier.") | ||
|
|
||
| # Index packages have their structure defined by package.json, | ||
| # so rename_to_init is not applicable |
There was a problem hiding this comment.
The comment says "rename_to_init is not applicable" for index packages, but the code then sets use_rename_to_init = True (line 115). This seems contradictory. If rename_to_init is not applicable, why set it to True? Consider clarifying the comment to explain that index packages handle their own structure, so the flag is set to True (the default/safe value) to avoid interfering with package.json-defined behavior.
| # so rename_to_init is not applicable | |
| # so rename_to_init is ignored for them. We set use_rename_to_init = True | |
| # as a safe default to avoid interfering with package.json-defined behaviour. |
belay/cli/add.py
Outdated
| # Try local path (absolute or relative) | ||
| if uri.startswith("/") or uri.startswith("./") or uri.startswith("../"): |
There was a problem hiding this comment.
The path detection only handles Unix-style paths. On Windows, absolute paths typically start with drive letters (e.g., C:\path\to\file) which won't match these conditions. Consider using Path(uri).is_absolute() or checking for Windows-style paths with uri[1:3] == ':\\' to improve cross-platform compatibility.
| # Try local path (absolute or relative) | |
| if uri.startswith("/") or uri.startswith("./") or uri.startswith("../"): | |
| # Try local path (absolute or relative, cross-platform) | |
| if Path(uri).is_absolute() or uri.startswith("./") or uri.startswith("../"): |
| return uri | ||
|
|
||
|
|
||
| def _check_dependency_not_exists(pyproject_path: Path, package: str, group: str): |
There was a problem hiding this comment.
Missing return type annotation. The function should specify -> None as it doesn't return a value.
| def _check_dependency_not_exists(pyproject_path: Path, package: str, group: str): | |
| def _check_dependency_not_exists(pyproject_path: Path, package: str, group: str) -> None: |
| group: str, | ||
| develop: bool, | ||
| rename_to_init: bool, | ||
| ): |
There was a problem hiding this comment.
Missing return type annotation. The function should specify -> None as it doesn't return a value.
| ): | |
| ) -> None: |
| def __init__(self): | ||
| self._cached_functions: list = [] | ||
| def __init__(self) -> None: | ||
| self._cached_functions: list[functools._lru_cache_wrapper] = [] |
There was a problem hiding this comment.
Using the private type functools._lru_cache_wrapper is not recommended as it's an internal implementation detail (indicated by the leading underscore). Private APIs can change without notice between Python versions. Consider using a more generic type like list[Any] or importing from typing if a more specific type is needed.
| self._cached_functions: list[functools._lru_cache_wrapper] = [] | |
| self._cached_functions: list[Callable[..., Any]] = [] |
| if develop or not rename_to_init: | ||
| # Need full dict specification | ||
| dep_value = tomlkit.inline_table() | ||
| dep_value["uri"] = uri | ||
| if develop: | ||
| dep_value["develop"] = True | ||
| if not rename_to_init: | ||
| dep_value["rename_to_init"] = False | ||
| else: | ||
| # Simple string format (most common case) | ||
| dep_value = uri |
There was a problem hiding this comment.
The logic for building dependency values is duplicated between _build_dependency_value (lines 154-161) and this function (lines 268-278). If the dependency value construction logic needs to change, it must be updated in both places. Consider calling _build_dependency_value here and converting its result to tomlkit format, or extracting the shared logic into a single helper function.
|
|
||
| # Handle git provider URLs (shorthand and HTTPS, including repo root URLs) | ||
| try: | ||
| parsed = GitProviderUrl.parse(uri) |
There was a problem hiding this comment.
The version suffix is split from the URI but then the original uri (with version suffix) is passed to GitProviderUrl.parse() instead of base_uri. While this works because GitProviderUrl treats @ as a branch specifier, it's confusing. Either use base_uri here if version splitting is needed, or remove the split_version_suffix call on line 39 if it's not needed for git URLs. This inconsistency makes the code harder to understand.
| parsed = GitProviderUrl.parse(uri) | |
| parsed = GitProviderUrl.parse(base_uri) |
| ) | ||
|
|
||
|
|
||
| def _build_dependency_value(uri: str, develop: bool, rename_to_init: bool): |
There was a problem hiding this comment.
Missing return type annotation. The function should specify its return type (Union[str, Dict[str, Any]] or similar) for better type safety and IDE support.
Adds a new CLI command
belay addthat adds dependencies topyproject.toml. Addresses #203.Usage
Supported URI Formats