Skip to content

[none]: refactor connection#5

Open
tayloraswift wants to merge 3 commits into
masterfrom
refactor-connection
Open

[none]: refactor connection#5
tayloraswift wants to merge 3 commits into
masterfrom
refactor-connection

Conversation

@tayloraswift
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the GitHub.Client.Connection by introducing a shared fetch helper to handle HTTP requests, redirects, and rate limiting for both REST and GraphQL calls. Several critical issues were identified in the review: the internal fetch method requires the @usableFromInline attribute to be compatible with public inlinable methods, the GraphQL post implementation currently fails to send the required query in the request body, and a regression was introduced by hardcoding the RateLimitError type, which breaks generic support.

Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift
Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift
Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift Outdated
Co-authored-by: Copilot <copilot@github.com>
@tayloraswift
Copy link
Copy Markdown
Member Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the GitHub.Client.Connection to use a shared internal fetch method for both REST and GraphQL requests, improving code reuse and handling redirects and rate limits consistently. Feedback includes marking the fetch method as @usableFromInline to support its use in public @inlinable methods, fixing a logic error where the HTTP status code might not be captured correctly during redirects, and maintaining consistency by using Void instead of () for generic specialization.

Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift
Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift
Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift Outdated
Comment thread Sources/GitHubClient/GitHub.Client.Connection.swift
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.

1 participant