Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The separator-rejection logic is correct for both POSIX and Windows modules, is thoroughly covered by the added regression tests, and includes accurate documentation and a release note.
No open issues 🎉
What changed in this PR
This PR fixes a bug in the path library's Path.with_suffix, where a replacement suffix containing a directory separator (e.g. .txt/child) could silently change the parent directory (archive.tar.gz → archive.tar.txt/child). The fix extends the existing ValueError validation to reject the configured path module's sep and altsep, correctly distinguishing POSIX (where backslash is a valid filename character) from Windows. This aligns behavior with modern pathlib.
Changes:
- Reject suffixes containing
module.sep/module.altsepinwith_suffix, reusing the existingValueErrorconvention and preserving the empty-suffix behavior. - Update the
with_suffixdocstring to state the suffix must contain no directory separators. - Add parametrized regression tests (separators at start/middle/end, POSIX backslash preservation, directory preservation, return types) plus a towncrier bugfix news fragment.
| File | Description |
|---|---|
path/__init__.py |
Adds separator rejection to with_suffix using the path's configured module, and documents the constraint. |
tests/test_path.py |
Adds parametrized tests for separator rejection across posixpath/ntpath, POSIX backslash preservation, and parent-directory preservation. |
newsfragments/+suffix-separators.bugfix.rst |
Adds the towncrier bugfix release note describing the change. |
I reviewed the separator logic (the sep and sep in suffix guard correctly handles posixpath.altsep being None), confirmed self.module, stripext, and the / operator (__truediv__ → joinpath) behave as the tests assume, and verified the news fragment follows the skeleton's towncrier conventions. The change is small, correct, and well covered by the added tests.
Review effort: Balanced
Path('archive.tar.gz').with_suffix('.txt/child')currently returnsarchive.tar.txt/child: replacing an extension changes the parent directory. Windows-style paths also accept a backslash in the suffix.Reject the configured path module's
sepandaltsepusing the existingValueErrorconvention. POSIX backslashes remain valid filename characters. Add regression tests covering separators at the start, middle and end of the suffix, valid compound extensions, parent-directory preservation and return types, plus a release note.This is separate from #253, which permits an empty suffix; this patch preserves the existing empty-suffix behavior.
Validation:
ntpathandposixpathcases on the same host.tox -e py -- -qenvironment passes 178 checks, 1 skipped, including doctests, type checking, lint/format checks, package metadata and coverage (99% as reported by the configured plugins). The same tox environment was run using a task-local work directory.git diff --checkpass. The complete Windows run is limited by this machine's missing symlink-creation privilege (WinError 1314in symlink-related tests); no permissions or test settings were changed to bypass it.AI disclosure: this contribution was prepared and validated with OpenAI Codex assistance. No human manual review is claimed.