Skip to content

Conversation

@mattt
Copy link
Collaborator

@mattt mattt commented Dec 5, 2025

This PR applies the changes made by huggingface/swift-transformers#295 to all places in this package where URLs are constructed from templated paths.

mattt added 2 commits December 5, 2025 05:15
…ments

Use appending(component:) for revision values that may contain slashes (e.g., pr/1 → pr%2F1)
Copilot finished reviewing on behalf of mattt December 5, 2025 13:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors URL construction throughout the HuggingFace Hub client to use URL.appending(path:) and URL.appending(component:) methods instead of string interpolation, following the pattern introduced in PR #295. The changes improve URL safety by ensuring proper encoding of dynamic path components.

Key Changes

  • Added URL-based overloads for fetch, fetchStream, and fetchData methods in HTTPClient to support direct URL construction
  • Replaced string-based path construction with URL builder methods across all Hub client extensions (Spaces, Models, Datasets, Repos, Organizations, Files)
  • Updated error messages to be more generic when URL construction fails

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
Sources/HuggingFace/Shared/HTTPClient.swift Added URL-based method overloads and refactored common logic into private helper methods; updated error message for URL construction failures
Sources/HuggingFace/Hub/HubClient+Spaces.swift Converted all API paths from string interpolation to use appending(path:) for static segments and appending(component:) for dynamic values like revisions
Sources/HuggingFace/Hub/HubClient+Repos.swift Updated repository settings API path construction to use URL builder methods with kind.pluralized
Sources/HuggingFace/Hub/HubClient+Organizations.swift Converted organization resource groups API path from string interpolation to URL builder pattern
Sources/HuggingFace/Hub/HubClient+Models.swift Refactored all model API endpoints to use URL builder methods consistently for paths and components
Sources/HuggingFace/Hub/HubClient+Files.swift Updated file operation paths to use URL builder methods; inconsistently uses repo.description instead of separate namespace/name components; includes unrelated symlink resolution change
Sources/HuggingFace/Hub/HubClient+Datasets.swift Converted all dataset API paths from string interpolation to use URL builder methods consistently
Comments suppressed due to low confidence (1)

Sources/HuggingFace/Hub/HubClient+Files.swift:272

  • This symlink resolution change (lines 269-270, 272) appears to be unrelated to the PR's stated purpose of using appending(path:) and appending(component:) consistently for URL construction. While this may be a valid fix, it should ideally be in a separate PR with its own description and justification, or at minimum be mentioned in the PR description.

The comment change from "Copy from cache to destination" to "Copy from cache to destination (resolve symlinks first)" and the addition of let resolvedPath = cachedPath.resolvingSymlinksInPath() represents a behavioral change in how cached files are handled.

            // Copy from cache to destination
            try? FileManager.default.removeItem(at: destination)
            try FileManager.default.copyItem(at: cachedPath, to: destination)
            progress?.completedUnitCount = progress?.totalUnitCount ?? 100

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 278 to 282
.appending(path: repo.description)
.appending(path: endpoint)
.appending(component: revision)
.appending(path: repoPath)
var request = try await httpClient.createRequest(.get, url: url)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: endpoint)
    .appending(component: revision)
    .appending(path: repoPath)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Copilot uses AI. Check for mistakes.
let url = httpClient.host
.appending(path: "api")
.appending(path: kind.pluralized)
.appending(path: repo.description)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: "api")
    .appending(path: kind.pluralized)
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: "commit")
    .appending(component: branch)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Suggested change
.appending(path: repo.description)
.appending(path: repo.namespace)
.appending(path: repo.name)

Copilot uses AI. Check for mistakes.
let url = httpClient.host
.appending(path: "api")
.appending(path: kind.pluralized)
.appending(path: repo.description)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: "api")
    .appending(path: kind.pluralized)
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: "tree")
    .appending(component: revision)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Suggested change
.appending(path: repo.description)
.appending(path: repo.namespace)
.appending(path: repo.name)

Copilot uses AI. Check for mistakes.
Comment on lines 520 to 524
.appending(path: repo.description)
.appending(path: "resolve")
.appending(component: revision)
.appending(path: repoPath)
var request = try await httpClient.createRequest(.head, url: url)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: "resolve")
    .appending(component: revision)
    .appending(path: repoPath)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Copilot uses AI. Check for mistakes.
let url = httpClient.host
.appending(path: "api")
.appending(path: kind.pluralized)
.appending(path: repo.description)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format, which when appended as a path, treats the / as part of the path segment.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: "api")
    .appending(path: kind.pluralized)
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: "upload")
    .appending(component: branch)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Suggested change
.appending(path: repo.description)
.appending(path: repo.namespace)
.appending(path: repo.name)

Copilot uses AI. Check for mistakes.
let urlPath = "/\(repo)/\(endpoint)/\(revision)/\(repoPath)"
var request = try await httpClient.createRequest(.get, urlPath)
let url = httpClient.host
.appending(path: repo.description)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Using repo.description with .appending(path:) is inconsistent with the pattern used elsewhere in the codebase. The repo.description returns "namespace/name" format.

For consistency with the pattern established in HubClient+Spaces.swift, HubClient+Models.swift, and HubClient+Datasets.swift, this should be split into separate components:

let url = httpClient.host
    .appending(path: repo.namespace)
    .appending(path: repo.name)
    .appending(path: endpoint)
    .appending(component: revision)
    .appending(path: repoPath)

This ensures proper handling of namespace and name components and maintains consistency across the codebase.

Suggested change
.appending(path: repo.description)
.appending(path: repo.namespace)
.appending(path: repo.name)

Copilot uses AI. Check for mistakes.
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