-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
base: main
Are you sure you want to change the base?
Users typed endpoint #30065
Conversation
0bd51b8
to
db8605f
Compare
zerver/views/users.py
Outdated
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))] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
), | ||
] | ||
] | ||
] = None, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.pydantic.dev/2.0/usage/types/number_types/ might offer some inspiration.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
db8605f
to
c8dc5a4
Compare
Migrated
users
anduser_topics
to typed endpoint.I have not used Pydantic's
URL
type forpayload_url
becauseURL
doesn't flag incomplete URLs likehttp://127.0..:6000
.I have also created
check_int_param_in
which is similar tocheck_int_in
butcheck_int_param_in
raises aValueError
notValidationError
.Similarly
check_param_url
, which is similar tocheck_url
.