-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Make annotation for methods
more permissive
#2903
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
Conversation
starlette/routing.py
Outdated
@@ -211,7 +211,7 @@ def __init__( | |||
path: str, | |||
endpoint: typing.Callable[..., typing.Any], | |||
*, | |||
methods: list[str] | None = None, | |||
methods: typing.Collection[str] | None = 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.
We typically use typing.Sequence
across the project.
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.
Indeed, but in this case it makes sense to allow sets in addition to list and tuples
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.
Reminder: https://docs.python.org/3/library/typing.html#deprecated-aliases
Shouldn't we start applying new standards?
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.
I followed the current convention of importing everything from the typing module, but if #2867 gets merged before this PR, then I'll adjust 😄
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.
I changed to import from collections.abc
Summary
The type annotation for
methods
in theRoute
constructor restricts the input to lists of strings when it can accept any iterable. Themethods
on theRoute
itself are stored as a set.I had the issue when I was trying to build new routes from other preexisting routes:
I changed the annotation to
Collection
, though the current implementation would work with anyIterable
.Collection
allows lists, sets, tuples, andIterable
would also allow generators. Let me know what you prefer.Checklist