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 URLPattern options to django.url.conf since they were missing. #583

Merged
merged 5 commits into from
Apr 10, 2021
Merged

Conversation

LanDinh
Copy link
Contributor

@LanDinh LanDinh commented Apr 5, 2021

I have made things!

It's possible to pass in List[URLPattern] as argument to django.urls.include, which then results in List[URLPattern] as output of that function. The current type hints for all of django.urls.include, django.urls.path and django.urls.re_path don't support this use case - this PR adjust their type hints to support it.

Related issues

Fixes #582

@LanDinh
Copy link
Contributor Author

LanDinh commented Apr 5, 2021

Some follow-up work might be necessary on URLResolver, specifically the urlconf_name field - despite its name, it can also receive List[URLResolver] and List[URLPattern] as input (just look at the comments of the constructor in the Django implementation!), but I'm not sure what the exact full type / options are and don't want to mess up stuff, so I've left it out of this PR.

I took a quick look at the tests; it's not quite straightforward (I've never written any type checking test before). Is there some documentation somewhere that I could refer to on how to specify the expected output of cases? E. g. tests to make sure that List[URLPattern] will be expected and not raise a type error.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

Can we please test that now it works?

@LanDinh
Copy link
Contributor Author

LanDinh commented Apr 10, 2021

I've tested it with my local snippet, it works :) Am now looking into writing a test for that.

@LanDinh
Copy link
Contributor Author

LanDinh commented Apr 10, 2021

Test added.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@sobolevn sobolevn merged commit ceb08f1 into typeddjango:master Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Type hints for django.urls.conf.* are incorrect
2 participants