Skip to content

Use pathSegment(_:) for the remaining inline path encodings #112

Open

Description

@Adron

Noted while doing #95 (see PR #111), which collapsed the query-value encoders. The path side has the
same shape and was deliberately left alone to keep that PR a single concern.

What's there

APIClientTransport.swift provides:

func pathSegment(_ segment: String) -> String {
    segment.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? segment
}

but roughly a dozen call sites still inline the same expression:

let encoded = id.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? id

Why this is lower priority than #95 was

These are correct as written. Unlike the query-value case, there is no latent bug here —
.urlPathAllowed is the right set for a path segment, and the inline form produces exactly what
pathSegment(_:) would. This is consistency and discoverability, not correctness.

The reason it is worth doing at all: with two near-identical idioms in the file, the next person
copying one has a coin-flip chance of reaching for the wrong one, and picking .urlQueryAllowed for
a query value is precisely the bug #95 just spent a PR removing. #111 put queryValue(_:) directly
beside pathSegment(_:) with a comment saying they are not interchangeable; converting the
stragglers makes the file self-consistent so the comment is reinforced rather than contradicted.

Build this

  • Replace inline .urlPathAllowed encodings of a path segment with pathSegment(_:).
  • Leave anything that is not a single path segment alone — if a call site is encoding a whole
    path or something with intentional slashes, pathSegment(_:) is the wrong tool and it should
    keep doing what it does, with a comment saying why.
  • No behaviour change. If any conversion changes a single byte on the wire, stop and say so —
    that would mean the two forms were not equivalent after all.

Acceptance criteria

  • No inline .urlPathAllowed path-segment encoding remains, or each survivor has a one-line
    comment explaining why it is not using the helper.
  • Suite green with no test changes needed — a pure refactor should not require them.

Files: Services/APIClient.swift and the APIClient+*.swift extensions.

Activity

  1. added
    tech-debtCleanup / dead code / correctness hygiene
    ws:papercutsW8 — small parity papercuts
    on Sep 17, 2026
  2. Adron commented on Sep 17, 2026

    @Adron
    MemberAuthor

    Correcting the size estimate in the description

    I wrote "roughly a dozen call sites", carrying over the estimate from #111's report. Counted
    properly on main:

    File Inline .urlPathAllowed encodings
    APIClient.swift 92
    ILWebURL.swift 3
    APIClient+Documents.swift 1
    APIClient+Organizations.swift 1
    APIClient+ListContributors.swift 1
    APIClient+GitHub.swift 1
    APIClient+Lists.swift 1
    APIClientTransport.swift 1 (the helper's own definition — leave it)

    So ~100, not ~12. That changes the character of the task: it is a large mechanical sweep rather than
    a quick tidy, and APIClient.swift alone will produce a diff that conflicts with anything else in
    flight in that file.

    Sequencing suggestion: run it when nothing else is touching APIClient.swift, and ideally as one
    commit that is purely mechanical, so a reviewer can scan it as find-and-replace rather than reading
    92 individual changes.

    ILWebURL.swift is worth a second look rather than blind conversion — it builds web URLs rather
    than API request paths, so it may not have access to the transport helper at all, and its three sites
    may be correct exactly as they are.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    tech-debtCleanup / dead code / correctness hygienews:papercutsW8 — small parity papercuts

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions