Restructure the module files #16
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR restructures the URLPath module from a monolithic single-file design to a more maintainable multi-module architecture. The main purpose is to improve code organization and maintainability by separating concerns into dedicated modules.
Key changes:
- Split the large
urlpath/__init__.pyinto separate modules for utilities, URL classes, and pathlib flavour logic - Simplified the main
__init__.pyto serve as a clean public API entry point - Updated the Makefile to consolidate formatting and linting commands
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
urlpath/__init__.py |
Converted to a clean public API module that imports from submodules |
urlpath/_utils.py |
New module containing utility classes (FrozenDict, cached_property, etc.) |
urlpath/_url.py |
New module containing the main URL and JailedURL class implementations |
urlpath/_flavour.py |
New module containing the custom pathlib flavour for URL parsing |
Makefile |
Consolidated format and lint commands into simpler fix and check targets |
|
|
||
| from __future__ import annotations | ||
|
|
||
| __all__ = ("FrozenDict", "FrozenMultiDict", "MultiDictMixin", "cached_property", "netlocjoin", "_url_splitroot") |
There was a problem hiding this comment.
The all tuple includes internal functions like _url_splitroot which are prefixed with underscore to indicate they're private. Consider removing private functions from all or making them properly public if they need to be exported.
| __all__ = ("FrozenDict", "FrozenMultiDict", "MultiDictMixin", "cached_property", "netlocjoin", "_url_splitroot") | |
| __all__ = ("FrozenDict", "FrozenMultiDict", "MultiDictMixin", "cached_property", "netlocjoin") |
| combined = cls._combine_args(canonicalized_args) | ||
| return super().__new__(cls, *combined) |
There was a problem hiding this comment.
The condition len(canonicalized_args) > 1 is used in multiple places with identical logic. Consider extracting this into a helper method to reduce code duplication and improve maintainability.
| if len(canonicalized_args) > 1: | ||
| combined = type(self)._combine_args(canonicalized_args) # type: ignore[attr-defined] | ||
| super().__init__(*combined) |
There was a problem hiding this comment.
This duplicates the same multi-argument combination logic from new. The duplication could lead to inconsistencies if the logic needs to change. Consider extracting this pattern into a shared helper method.
| has_drv = True # drive is scheme + netloc | ||
| is_supported = True # supported in all platform | ||
|
|
||
| def splitroot(self, part: str, sep: str = _PosixFlavour.sep) -> tuple[str, str, str]: |
There was a problem hiding this comment.
Using _PosixFlavour.sep directly in the default parameter creates a tight coupling and could cause issues if _PosixFlavour is None (as it is in Python 3.12+). Consider using a string literal / instead for consistency with the Python 3.12+ version.
| def splitroot(self, part: str, sep: str = _PosixFlavour.sep) -> tuple[str, str, str]: | |
| def splitroot(self, part: str, sep: str = "/") -> tuple[str, str, str]: |
No description provided.