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

RuleFactories have incorrect type annotations #2183

Closed
jkhsjdhjs opened this issue Jul 7, 2021 · 2 comments · Fixed by #2185
Closed

RuleFactories have incorrect type annotations #2183

jkhsjdhjs opened this issue Jul 7, 2021 · 2 comments · Fixed by #2185
Labels
Milestone

Comments

@jkhsjdhjs
Copy link
Contributor

The __init__() functions of most RuleFactories in routing.py have incorrect type annotations. The classes in question are Subdomain, Submount, EndpointPrefix and RuleTemplateFactory. All respective __init__() methods have the type of their rules parameter declared as Iterable[Rule], when it should be Iterable[RuleFactory].

I noticed this when upgrading werkzeug to 2.0.0 as this update introduced type annotations. My Map contains several Submounts which in turn contain other Submounts, which works perfectly fine. The new type annotations, however, don't allow this as Submounts aren't Rules:

aas/adapter/http.py:334: error: List item 0 has incompatible type "Submount"; expected "Rule"

Why I believe these type annotations are incorrect:

The get_rules() method of these RuleFactories contains the following loop:

for rulefactory in self.rules:

The naming here makes it pretty clear to me that self.rules (which is assigned directly from the rules parameter of the __init__() method) contains rule factories and not just rules. Also there would be no usecase for a get_rules() function if self.rules already only contained rules.
This and the fact that my code, in which I use nested RuleFactories, works, lead me to believe that the type annotation here are indeed wrong.

If confirmed, I'd be happy to provide a PR to fix this issue.

@davidism davidism added the typing label Jul 8, 2021
@davidism
Copy link
Member

davidism commented Jul 8, 2021

I believe I used Rule because that's the much more common case, and RuleFactory has other APIs that sort of make this type cascade. Can't really remember, but I recall it being less clear with the RuleFactory. PRs welcome though if you want to explore what it would look like.

@jkhsjdhjs
Copy link
Contributor Author

I've opened #2185 in an attempt to fix this issue by simply changing the type annotations. This fixes the issue for my usecase of nested RuleFactories. I didn't fully understand what you meant with

RuleFactory has other APIs that sort of make this type cascade

so I hope I didn't miss anything crucial here.

@davidism davidism added this to the 2.0.2 milestone Aug 6, 2021
@davidism davidism linked a pull request Aug 6, 2021 that will close this issue
6 tasks
@davidism davidism closed this as completed Aug 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants