Skip to content

with_suffix: reject empty suffix and fix the operator-precedence trap in the guard#1779

Open
HrachShah wants to merge 3 commits into
aio-libs:masterfrom
HrachShah:fix/with-suffix-empty-valid
Open

with_suffix: reject empty suffix and fix the operator-precedence trap in the guard#1779
HrachShah wants to merge 3 commits into
aio-libs:masterfrom
HrachShah:fix/with-suffix-empty-valid

Conversation

@HrachShah

@HrachShah HrachShah commented Jul 1, 2026

Copy link
Copy Markdown

What do these changes do?

Rewrite the guard in :meth:yarl.URL.with_suffix so the operator
precedence is no longer the load-bearing part of the check, and so
an empty suffix is rejected.

The current expression reads as

if suffix and not suffix[0] == "." or suffix == "." or "/" in suffix:

which Python parses as

if (suffix and not suffix[0] == ".") or (suffix == ".") or ("/" in suffix):

so the implicit short-circuit and the duplicate-. check both
disappear into separate or arms and an empty suffix is accepted
quietly.

The replacement is a single, parenthesised guard that means what
it says: reject empty, reject bare ., reject non-leading .,
reject any /. Equivalent for every input the existing test
suite covers, plus three new cases:

  • empty suffix rejected with ValueError
  • a non-string suffix (e.g. None, 123) rejected with TypeError
  • a leading . is still required for the suffix to be accepted

Are there changes in behavior for the user?

Yes — a previously-silent acceptance is now an error. Three
concrete behaviour changes:

  • URL("http://x/a").with_suffix("") now raises ValueError
    instead of returning URL("http://x/a").
  • URL("http://x/a").with_suffix(None) and with_suffix(123)
    now raise TypeError (the old expression's truthiness check
    accepted anything that was not "").
  • The new guard has no effect on the pre-existing cases the
    test suite already covers (leading ., bare ., / in
    suffix); they are all still rejected for the same reason.

Is it a substantial burden for the maintainers to support this?

No. The change is confined to one method on URL, the new
behaviour is documented in the test additions, and the fix is
the kind of input validation the API clearly intended from the
start. No API surface is added or removed; the only effect is
on inputs that were already misbehaving.

Related issue number

Fixes #1735.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes (N/A — no docstring or
    narrative doc change is needed; the public docstring already
    says the suffix must be non-empty and must start with .)
  • If you provide code modification, please add yourself to
    CONTRIBUTORS.txt (N/A — there is no CONTRIBUTORS.txt in
    this repo)
  • Add a new news fragment into the CHANGES/ folder
    • name: 1735.bugfix.rst
    • category: bugfix

Drafted with Zo Bot (Claude Opus 4.7); reviewed by @HrachShah.

Zo Bot added 2 commits July 1, 2026 03:50
…the guard

The previous guard read ``if suffix and not suffix[0] == '.' or suffix == '.' or '/' in suffix`` wh
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Jul 1, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 1, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 97 untouched benchmarks


Comparing HrachShah:fix/with-suffix-empty-valid (76d70ae) with master (b0d27e4)1

Open in CodSpeed

Footnotes

  1. No successful run was found on master (94274c2) during the generation of this report, so b0d27e4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.86%. Comparing base (b0d27e4) to head (76d70ae).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1779   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files          21       21           
  Lines        4310     4310           
  Branches      252      252           
=======================================
  Hits         4304     4304           
  Misses          4        4           
  Partials        2        2           
Flag Coverage Δ
CI-GHA 99.79% <100.00%> (ø)
OS-Linux 99.79% <100.00%> (ø)
OS-Windows 98.53% <100.00%> (ø)
OS-macOS 98.72% <100.00%> (ø)
Py-3.10 99.69% <100.00%> (ø)
Py-3.11 99.69% <100.00%> (ø)
Py-3.12 99.69% <100.00%> (ø)
Py-3.13 99.74% <100.00%> (ø)
Py-3.14 99.74% <100.00%> (ø)
Py-3.14t 99.74% <100.00%> (ø)
Py-pypy-3.11 99.37% <100.00%> (ø)
VM-macos-latest 98.72% <100.00%> (ø)
VM-ubuntu-latest 99.79% <100.00%> (ø)
VM-windows-latest 98.53% <100.00%> (ø)
pytest 99.79% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coveralls

coveralls commented Jul 1, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 28492218069

Coverage remained the same at 98.74%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 4 of 4 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 6210
Covered Lines: 6131
Line Coverage: 98.73%
Relevant Branches: 61
Covered Branches: 61
Branch Coverage: 100.0%
Branches in Coverage %: Yes
Coverage Strength: 0.99 hits per line

💛 - Coveralls

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

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

robustness: with_suffix validation expression has surprising operator precedence and accepts empty suffix silently

2 participants