Skip to content

Add 4 organization endpoints #3

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Eilandis
Copy link

@Eilandis Eilandis commented Nov 22, 2023

[IMP] This PR implements 4 endpoints related to the Clerk organizations :

[FIX] Also add "Content-type" : "application/json" in the HTTP request header, because it is now required for every clerk API call.

pyproject.toml Outdated
aiohttp = {extras = ["speedups"], version = "^3.7.3"}
pydantic = "^1.7.3"
python = "^3.11"
aiohttp = "3.9.0"
Copy link
Owner

Choose a reason for hiding this comment

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

is there a reason this needs to be pinned to exactly 3.9.0?

Copy link
Author

Choose a reason for hiding this comment

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

I had to use a specific beta version during a few weeks (3.9.0b0), because of the 3.11 python version I was using.
When it was finally included in the 3.9.0, I forgot to put it back.
I'll move it back to : aiohttp = {extras = ["speedups"], version = "^3.9.0"}


async def list(self) -> List[types.Organization]:
"""Retrieve a list of all organizations"""
async with self._client.get(self.endpoint) as r:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
async with self._client.get(self.endpoint) as r:
async with self._client.get(self.endpoint) as resp:

let's use resp instead of r for all of these methods

Copy link
Owner

Choose a reason for hiding this comment

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

Please update the rest of the methods and make sure the code still works. There's still a reference to s rather than resp_json

Copy link
Author

@Eilandis Eilandis Nov 28, 2023

Choose a reason for hiding this comment

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

Sorry about that, should have tested it.
Not a big user of one-letter vars myself, I kept them originally to be "coherent" with your methods in clients.py

I fixed this issues in my last commit.

@Eilandis Eilandis force-pushed the clerk-sdk-python-fork branch from 812c864 to b2a16a0 Compare November 27, 2023 17:33
@Eilandis Eilandis requested a review from tizz98 November 27, 2023 17:38
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