Skip to content

Conversation

@litteratum
Copy link
Owner

No description provided.

@litteratum litteratum requested a review from Copilot November 16, 2025 10:13
@litteratum litteratum self-assigned this Nov 16, 2025
@litteratum litteratum linked an issue Nov 16, 2025 that may be closed by this pull request
@litteratum litteratum merged commit 3c1cb47 into master Nov 16, 2025
8 of 9 checks passed
@litteratum litteratum deleted the 18-more-robust-urljoin branch November 16, 2025 10:16
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 makes the endpoint parameter optional in the API client's _request method and switches from string concatenation to urllib.parse.urljoin for URL construction. This allows API calls to be made directly to the base URL without requiring an endpoint.

Key Changes:

  • Made the endpoint parameter optional (defaults to None) in the _request method
  • Replaced string concatenation with urllib.parse.urljoin for building URLs
  • Added comprehensive test coverage for the case when no endpoint is provided

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
httprest/api.py Made endpoint optional and switched to urljoin for URL construction
tests/unit/test_api.py Added parametrized test to verify behavior when endpoint is empty or None
tests/unit/fakes.py Updated test API to support optional endpoint parameter with proper type annotations

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


def _build_url(self, endpoint: str) -> str:
return f"{self._base_url}/{endpoint.strip('/')}"
def _build_url(self, endpoint: _Optional[str]) -> str:
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

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

urllib.parse.urljoin() requires both arguments to be strings, but endpoint can now be None. This will cause a TypeError when endpoint=None is passed. The function should handle the None case, for example:

def _build_url(self, endpoint: _Optional[str]) -> str:
    if endpoint is None:
        return self._base_url
    return urllib.parse.urljoin(self._base_url, endpoint)

Additionally, note that urljoin("http://fake.com", "") returns "http://fake.com" (without trailing slash), which matches the expected behavior in the test.

Suggested change
def _build_url(self, endpoint: _Optional[str]) -> str:
def _build_url(self, endpoint: _Optional[str]) -> str:
if endpoint is None:
return self._base_url

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.

More robust urljoin

2 participants