-
Notifications
You must be signed in to change notification settings - Fork 6
Add python 3.12 support #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds Python 3.12 support to the URLPath library by adapting to pathlib's architectural changes in Python 3.12+, where the _PosixFlavour class was removed and replaced with a module-based approach.
- Implements version-specific compatibility for pathlib API changes in Python 3.12+
- Adds custom
_URLFlavourclass and modified URL initialization logic for Python 3.12 compatibility - Updates project configuration to include Python 3.12 in supported versions and CI testing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| urlpath/init.py | Implements Python 3.12 compatibility with new flavour class, modified URL initialization, and version-specific handling of pathlib changes |
| pyproject.toml | Adds Python 3.12 classifier to supported versions |
| .github/workflows/test.yml | Adds Python 3.12 to CI test matrix |
| Makefile | Adds UV_LINK_MODE export to avoid filesystem reflink issues |
|
|
||
| # Python 3.12+ removed _PosixFlavour class, replaced with module-based approach | ||
| if sys.version_info >= (3, 12): | ||
| _PosixFlavour = None # noqa: F811 |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The assignment _PosixFlavour = None with # noqa: F811 is confusing. Consider using a more explicit approach like conditionally defining the variable or using a placeholder class to make the intent clearer.
| _PosixFlavour = None # noqa: F811 | |
| class _PosixFlavour: | |
| """Placeholder for _PosixFlavour (removed in Python 3.12+).""" | |
| pass |
urlpath/__init__.py
Outdated
| paths = tuple(paths[0]) | ||
| return self.sep.join(paths) # type: ignore[arg-type] |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The type ignore comment suggests a type mismatch. The logic handles both individual strings and a list, but the type annotation should reflect this flexibility or the implementation should be more type-safe.
| paths = tuple(paths[0]) | |
| return self.sep.join(paths) # type: ignore[arg-type] | |
| joinable: tuple[str, ...] = tuple(paths[0]) | |
| else: | |
| joinable = tuple(paths) # type: ignore | |
| return self.sep.join(joinable) |
| """ | ||
| if sys.version_info >= (3, 12): | ||
| # Canonicalize arguments for Python 3.12's stricter PurePath.__init__ | ||
| canonicalized_args = tuple(cls._canonicalize_arg(a) for a in args) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canonicalizing arguments twice (once in __new__ and again in __init__) for Python 3.12+ creates unnecessary overhead. Consider storing the canonicalized args or restructuring to avoid duplication.
| # In Python 3.12, super().name may have \x00 escape, clean it up | ||
| if sys.version_info >= (3, 12): | ||
| full_name = full_name.replace("\\x00", "/") |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The escape sequence cleanup logic is duplicated in multiple places (lines 417, 493, 735). Consider extracting this into a helper method to reduce duplication and improve maintainability.
| # - _drv = "" (empty, no URL scheme/netloc drive) | ||
| # - _root = "http://example.com/app" (the chroot as a fake filesystem root) | ||
| # - _tail_cached = ("path", "to", "content", "..", "file") | ||
| chroot_root_str = "".join(chroot._parts) # Join chroot parts into one string |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using \"\".join(chroot._parts) to create a root string seems unusual. This concatenates path components without separators, which may not produce the expected URL format. Consider using proper path joining or URL construction.
| chroot_root_str = "".join(chroot._parts) # Join chroot parts into one string | |
| chroot_root_str = str(chroot) # Use string representation of chroot |
b110073 to
7a151d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| def __new__(cls, *args: Any) -> URL: | ||
| """Create a new URL instance, canonicalizing arguments in Python 3.12+. | ||
| In Python 3.12, PurePath validation is stricter. We canonicalize arguments | ||
| (webob.Request, SplitResult, etc.) to strings before parent processing. | ||
| Args: | ||
| *args: URL components (strings, SplitResult, ParseResult, or webob.Request) | ||
| Returns: | ||
| New URL instance | ||
| """ | ||
| if sys.version_info >= (3, 12): | ||
| # Python 3.12: Canonicalize for stricter PurePath validation | ||
| # Note: This happens BEFORE _parse_args, so it's not redundant | ||
| canonicalized_args = tuple(cls._canonicalize_arg(a) for a in args) | ||
| return super().__new__(cls, *canonicalized_args) | ||
| else: | ||
| # Python < 3.12: No early validation, canonicalization happens in _parse_args | ||
| return super().__new__(cls, *args) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring mentions SplitResult and ParseResult types that are not handled in the _canonicalize_arg method. The method only handles webob.Request and converts everything else to string via str(). The docstring should be updated to accurately reflect what argument types are actually supported.
| # Python 3.12: Must canonicalize args again (__init__ gets original args) | ||
| canonicalized_args = tuple(self._canonicalize_arg(a) for a in args) | ||
| super().__init__(*canonicalized_args) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 390 states that init receives original args, but this creates redundant canonicalization. Consider refactoring to avoid double canonicalization by storing canonicalized args as an instance variable in new and reusing them in init.
| # Python 3.12: Must canonicalize args again (__init__ gets original args) | |
| canonicalized_args = tuple(self._canonicalize_arg(a) for a in args) | |
| super().__init__(*canonicalized_args) | |
| # Use canonicalized args from __new__ to avoid double canonicalization | |
| super().__init__(*getattr(self, "_canonicalized_args", args)) |
| def _from_parts(cls, args: Any) -> URL: | ||
| ret = super()._from_parts(args) | ||
| if sys.version_info >= (3, 12): | ||
| # Python 3.12 removed _from_parts, use direct construction | ||
| ret = cls(*args) | ||
| else: | ||
| ret = super()._from_parts(args) | ||
| ret._init() | ||
| return ret |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 450 is misleading. Python 3.12 did not remove _from_parts entirely - it's still available as an instance method. The comment should clarify that the method signature or behavior changed, not that it was removed.
| if hasattr(self, "_str"): | ||
| object.__delattr__(self, "_str") | ||
| if hasattr(self, "_tail_cached"): | ||
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts)) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line sets _tail_cached to chroot._parts, but chroot._parts is a list[str] and _tail_cached expects a tuple. While tuple() conversion handles this, the logic is incorrect - it should use chroot._parts[1:] to exclude the drive+root component, consistent with how _tail_cached is used elsewhere in the class.
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts)) | |
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts[1:])) |
e945135 to
ac5e176
Compare
ac5e176 to
b3f7d05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
| if sys.version_info >= (3, 12): | ||
| _PosixFlavour = None # noqa: F811 | ||
| else: | ||
| from pathlib import _PosixFlavour |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider using a more descriptive variable name than _PosixFlavour = None. Setting it to None with a noqa comment suggests this is only to satisfy linting, but it would be clearer to either omit this assignment or use a more explicit approach like try/except ImportError to handle the import conditionally.
| if sys.version_info >= (3, 12): | |
| _PosixFlavour = None # noqa: F811 | |
| else: | |
| from pathlib import _PosixFlavour | |
| try: | |
| from pathlib import _PosixFlavour | |
| except ImportError: | |
| _PosixFlavour = None |
| # Check if we have a cached value | ||
| if hasattr(self, "_parts_cache"): | ||
| return self._parts_cache # type: ignore[return-value] |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The type ignore comment suggests a type checking issue. Consider defining _parts_cache as an optional class attribute with proper typing to avoid the need for type ignores.
| try: | ||
| _ = self._tail_cached # type: ignore[attr-defined] | ||
| except AttributeError: |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The pattern of accessing _tail_cached just to trigger an AttributeError feels fragile. Consider checking if the attribute exists using hasattr(self, '_tail_cached') instead of relying on exception handling for control flow.
| try: | |
| _ = self._tail_cached # type: ignore[attr-defined] | |
| except AttributeError: | |
| if not hasattr(self, "_tail_cached"): |
| if hasattr(self, "_str"): | ||
| object.__delattr__(self, "_str") | ||
| if hasattr(self, "_tail_cached"): | ||
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts)) |
Copilot
AI
Oct 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 1598, chroot._parts is being accessed but should likely be chroot._parts[1:] to match the pattern used elsewhere in the code where the first element (drive+root) is excluded from tail components.
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts)) | |
| object.__setattr__(self, "_tail_cached", tuple(chroot._parts[1:])) |
Add python 3.12 support