Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small typing fixes #332

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Small typing fixes #332

merged 11 commits into from
Aug 13, 2024

Conversation

tribble
Copy link

@tribble tribble commented Aug 12, 2024

Description

Note: This is stacked on #331

OK, the branch name is a little LOL

This started as an attempt to fix some of the stricter validations from pylance, like shadowing the _http_client type. This led to a bit of a rabbit hole, but here was the outcome:

  • Extracted a client configuration protocol, the base modules do not need access to the client, just the base URL and client ID
  • The initialization of the base client, which would then initialize the HTTP based on the Generic type in was a bit confusing to me, so instead I directory initialize the HTTP client in the Sync or Async client. This eliminated the need for a generic in the base client entirely

I'm not married to the bigger changes I made around removing the generics for the BaseClient, but I feel like this is a little more direct and easier to understand.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@tribble tribble requested a review from mattgd August 12, 2024 23:34
Copy link
Contributor

@mattgd mattgd left a comment

Choose a reason for hiding this comment

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

I see this is still in draft, but left one comment.

Comment on lines +117 to +119
@property
def base_url(self) -> str:
return self._base_url
Copy link
Contributor

Choose a reason for hiding this comment

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

In base HTTP client we have a setter that enforces a trailing slash. Would be good to make sure these values are the same and perhaps enforce the trailing slash here instead:

@base_url.setter
def base_url(self, url: str) -> None:
"""Creates an accessible template for constructing the URL for an API request.
Args:
base_api_url (str): Base URL for api requests
"""
self._base_url = "{}{{}}".format(self._enforce_trailing_slash(url))

@tribble tribble marked this pull request as ready for review August 13, 2024 17:43
@tribble tribble requested a review from a team as a code owner August 13, 2024 17:43
@tribble tribble requested review from PaulAsjes and removed request for a team August 13, 2024 17:43
@tribble tribble merged commit 056d30b into beta-5.0 Aug 13, 2024
5 checks passed
@tribble tribble deleted the small-typing-fixes branch August 13, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants