Skip to content

Reject directory separators in replacement suffixes - #276

Open
Dcastroro wants to merge 1 commit into
jaraco:mainfrom
Dcastroro:fix/reject-directory-separators-in-suffix
Open

Dcastroro wants to merge 1 commit into
jaraco:mainfrom
Dcastroro:fix/reject-directory-separators-in-suffix

Conversation

@Dcastroro

Copy link
Copy Markdown

Path('archive.tar.gz').with_suffix('.txt/child') currently returns archive.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 sep and altsep using the existing ValueError convention. 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:

  • Against the original implementation, the new cases give 9 failed, 5 passed. With the fix, all 14 pass, including explicit ntpath and posixpath cases on the same host.
  • Linux, CPython 3.13.11: the configured tox -e py -- -q environment 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.
  • Windows, CPython 3.13.14: focused tox run passes 15 checks, including the configured type check; Ruff and git diff --check pass. The complete Windows run is limited by this machine's missing symlink-creation privilege (WinError 1314 in 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.

Copilot AI balanced review requested due to automatic review settings October 26, 2026 12:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.altsep in with_suffix, reusing the existing ValueError convention and preserving the empty-suffix behavior.
  • Update the with_suffix docstring 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants