types: Add StrPath typing, fix new_session
#597
Conversation
Reviewer's GuideIntroduces a new Class diagram for the new StrPath type aliasclassDiagram
namespace libtmux._internal.types {
class StrPath {
<<TypeAlias: str | os.PathLike~str~>>
}
}
Updated class diagram for the Session classclassDiagram
namespace libtmux.session {
class Session {
+new_window(window_name: str | None = None, start_directory: str | None = None, attach: bool = False, window_index: str = "", window_shell: str | None = None)
}
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #597 +/- ##
==========================================
+ Coverage 79.83% 79.84% +0.01%
==========================================
Files 22 23 +1
Lines 1914 1915 +1
Branches 294 294
==========================================
+ Hits 1528 1529 +1
Misses 266 266
Partials 120 120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/libtmux/session.py
Outdated
| @@ -587,7 +587,7 @@ def new_window( | |||
| self, | |||
| window_name: str | None = None, | |||
| *, | |||
| start_directory: None = None, | |||
| start_directory: str | None = None, | |||
There was a problem hiding this comment.
suggestion: Restrictive type annotation: consider accepting PathLike
Annotate start_directory as str | PathLike[str] | None (or use StrPath) to allow both strings and path-like objects, matching what pathlib.Path and .expanduser() accept.
Suggested implementation:
start_directory: str | os.PathLike[str] | None = None, start_directory = pathlib.Path(start_directory).expanduser()import os
src/libtmux/session.py
Outdated
| @@ -677,7 +677,7 @@ | |||
|
|
|||
| window_args += ("-P",) | |||
|
|
|||
| if start_directory: | |||
| if start_directory is not None: | |||
There was a problem hiding this comment.
question (bug_risk): Empty string now treated as a valid directory
Using is not None allows empty strings, which pathlib.Path interprets as the current directory. If this isn't intended, consider keeping the original truthiness check or explicitly rejecting empty strings.
| @@ -677,7 +677,7 @@ | |||
|
|
|||
| window_args += ("-P",) | |||
|
|
|||
| if start_directory: | |||
| if start_directory is not None: | |||
| # as of 2014-02-08 tmux 1.9-dev doesn't expand ~ in new-window -c. | |||
| start_directory = pathlib.Path(start_directory).expanduser() | |||
| window_args += (f"-c{start_directory}",) | |||
There was a problem hiding this comment.
suggestion (bug_risk): Unquoted directory argument may break on spaces
Passing the directory as an unquoted string may fail if the path contains spaces. Use separate arguments for the flag and value, or ensure proper quoting.
| @@ -0,0 +1,19 @@ | |||
| """Internal type annotations. | |||
There was a problem hiding this comment.
issue: Doc mentions StrOrBytesPath but alias is not defined
Please define the StrOrBytesPath alias or remove its mention from the docstring to prevent confusion.
| import typing as t | ||
|
|
||
| if t.TYPE_CHECKING: | ||
| from os import PathLike | ||
|
|
There was a problem hiding this comment.
suggestion: TypeAlias used outside TYPE_CHECKING import block
Since StrPath: TypeAlias is at the module level, ensure TypeAlias is available at runtime by importing it outside the TYPE_CHECKING block or move the alias declaration inside the block for clarity.
| import typing as t | |
| if t.TYPE_CHECKING: | |
| from os import PathLike | |
| import typing as t | |
| from typing import TypeAlias | |
| if t.TYPE_CHECKING: | |
| from os import PathLike | |
I'm making mistakes, but follow up to #597, #596 ## Summary This PR adds uniform `StrPath` type support for `start_directory` parameters across all methods in libtmux, enabling PathLike objects (like `pathlib.Path`) to be used alongside strings. ## Changes Made ### Type Annotations Updated - `Server.new_session`: `start_directory: str | None` → `start_directory: StrPath | None` - `Session.new_window`: `start_directory: str | None` → `start_directory: StrPath | None` - `Pane.split` and `Pane.split_window`: `start_directory: str | None` → `start_directory: StrPath | None` - `Window.split` and `Window.split_window`: `start_directory: str | None` → `start_directory: StrPath | None` ### Implementation Details - Added `StrPath` import to all affected modules - Updated docstrings to reflect "str or PathLike" support - Standardized logic patterns using `if start_directory:` (not `if start_directory is not None:`) to properly handle empty strings - Added path expansion logic with `pathlib.Path(start_directory).expanduser()` for proper tilde expansion ### Testing - Added comprehensive parametrized tests for all affected methods - Test cases cover: `None`, empty string, absolute path string, and `pathlib.Path` objects - Added separate pathlib-specific tests using temporary directories - Tests verify operations complete successfully with all input types - Integrated tests into existing test files following project conventions
Check if this works to fix https://github.com/tmux-python/libtmux/pull/596/files#r2106995968.
Summary by Sourcery
Add StrPath type alias for internal annotations and adjust new_window to properly handle optional string start_directory values.
New Features:
Bug Fixes: