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

Add type hints to management #497

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jun 11, 2023

Fixes #470.

This is ready for review, but I might take a second look, probably forgot a few things.

@Viicos Viicos requested a review from a team as a code owner June 11, 2023 19:40
@Viicos Viicos mentioned this pull request Jun 11, 2023
@adamjmcgrath
Copy link
Contributor

lgtm - thanks @Viicos

Just need to fix the build

@Viicos Viicos force-pushed the 470-type-hints-management branch from 516bae2 to 77a1c7c Compare July 7, 2023 11:34
@Viicos
Copy link
Contributor Author

Viicos commented Jul 7, 2023

@adamjmcgrath done (apart from the failing docs, I don't really know how to fix this), sorry for the delay

@adamjmcgrath
Copy link
Contributor

Thanks @Viicos - will take a look at the docs when I have a moment

@adamjmcgrath adamjmcgrath merged commit d2ab498 into auth0:master Jul 24, 2023
@adamjmcgrath adamjmcgrath mentioned this pull request Jul 25, 2023
include_totals: bool = True,
from_param: str | None = None,
take: int | None = None,
) -> list[dict[str, Any]]:
Copy link
Contributor

@shchotse shchotse Aug 2, 2023

Choose a reason for hiding this comment

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

pls, pay attention, here is the wrong return type, the all_organization_members method can also return the dict object if include_totals equals True or Pagination is enabled

{
  "members": [{"user_id": "xxxx", ...}, ...],
  "start": 0,
  "limit": 50,
  "total": n
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@shchotse This PR is merged, would you mind creating a separate issue? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shchotse thanks. As I said:

This is ready for review, but I might take a second look, probably forgot a few things.

I've tried specifying the correct return type where possible, but otherwise made use of Any. The issue is no OpenAPI file is available, so it's hard to be accurate. By the way some people suggested to auto generate the sdk from an OpenAPI spec, but this usually requires some additional work and the OpenAPI needs to be complete

include_totals: bool = True,
fields: List[str] | None = None,
include_fields: bool = True,
) -> List[dict[str, Any]]:

Choose a reason for hiding this comment

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

I believe this is the wrong type? I think it should be Dict[str, Any]

Copy link
Contributor

Choose a reason for hiding this comment

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

@lilidworkin This PR is merged, would you mind creating a separate issue? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lilidworkin thanks. See #497 (comment) for more details

@Viicos Viicos deleted the 470-type-hints-management branch August 2, 2023 15:46
@michaeloliverx
Copy link

Are there any plans to add some more concrete return types? E.g. using TypedDict

It would be a huge win for DX, currently I am maintaining local definitions and doing this:

from typing import NotRequired, TypedDict


class Role(TypedDict):
    id: str
    name: str
    description: str


class RolesResponse(TypedDict):
    """https://auth0.com/docs/api/management/v2/roles/get-roles"""
    start: int
    limit: int
    total: int
    roles: list[Role]

...

user_roles: RolesResponse = auth0_client.users.list_roles(user_id)

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.

Add type hints
5 participants