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

Users typed endpoint #30065

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

Conversation

kennethnrk
Copy link
Collaborator

Migrated users and user_topics to typed endpoint.

I have not used Pydantic's URL type for payload_url because URL doesn't flag incomplete URLs like http://127.0..:6000.

I have also created check_int_param_in which is similar to check_int_in but check_int_param_in raises a ValueError not ValidationError.

Similarly check_param_url, which is similar to check_url.

zerver/lib/validator.py Outdated Show resolved Hide resolved
short_name_raw: Annotated[str, ApiParamConfig("short_name")],
bot_type: Json[int] = UserProfile.DEFAULT_BOT,
payload_url: Json[
Annotated[str, AfterValidator(lambda x: check_param_url("payload_url", x))]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is there a better way to do this AfterValidator pattern that woudln't require the lambda x wrapper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For validation functions that accept only 1 positional argument we could pass the function directly to AfterValidator. But for functions like check_int_in which take a second parameter we would need tot use lambda or a similar wrapper function. Both lambda and wrapper functions have been used in the docs, the do the same thing esssentially.

zerver/views/users.py Outdated Show resolved Hide resolved
zerver/views/users.py Outdated Show resolved Hide resolved
),
]
]
] = None,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Any ideas on a better way to do this role parameter without so much nesting? I suppose an option would be to just declare a RoleParameterType and use that here? I think we have some other views that use a lot of enumerated integer parameters, so it'd be good to have a clean pattern for these.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Creating RoleParameterType is an option, I didn't create it initially because role isn't being used in any endpoint in any other file, but its being used twice here so it might be better to create a type.

We could use Enums or Literal but that would sacrifice the ability to pass a list. Both those would require us to hardcode specific values and do not take lists. Literal does take tuples but passing tuples throws linting errors.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yeah I think making a type for use in this file is worth it; if nothing else it'd be a good example pattern to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants