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

fix type hint for GenericSitemap(info_dict) #1111

Merged
merged 1 commit into from
Sep 2, 2022
Merged

fix type hint for GenericSitemap(info_dict) #1111

merged 1 commit into from
Sep 2, 2022

Conversation

djbrown
Copy link
Contributor

@djbrown djbrown commented Aug 17, 2022

Description

This PR softens the invariant type hint Dict to the covariant Mapping of django.contrib.sitemaps.GenericSitemap(info_dict).
See Python type hints: typing.Mapping vs. typing.Dict - Stack Overflow
Docs: The sitemap framework # GenericSitemap

Motivation

info_dict = {
    'queryset': Entry.objects.all()
}

urlpatterns = [
    path('sitemap.xml', sitemap,
         {'sitemaps': {'blog': GenericSitemap(info_dict)}},
         name='django.contrib.sitemaps.views.sitemap'),
]

Mypy error:

error: Argument 1 to "GenericSitemap" has incompatible type "Dict[str, _QuerySet[Entry, Entry]]"; expected "Dict[str, Union[datetime, _QuerySet[Model, Model], str]]"  [arg-type]
note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
note: Consider using "Mapping" instead, which is covariant in the value type

Test

In my local virtualenv I modified the file site-packages/django-stubs/contrib/sitemaps/__init__.pyi according to the change of this PR.
Afterwars ran the mypy check again and the errors was gone.

djbrown added a commit to djbrown/hbscorez that referenced this pull request Aug 17, 2022
Copy link
Contributor

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

LGTM, this will also allow other kinds of mappings if necessary.

@djbrown
Copy link
Contributor Author

djbrown commented Sep 2, 2022

@adamchainz is this mergable or am I missing something?

@adamchainz
Copy link
Contributor

I don't have merge rights, @sobolevn does.

@sobolevn
Copy link
Member

sobolevn commented Sep 2, 2022

Sorry, I've missed this PR somehow. Let's try it.

@adamchainz if you want to have merge rights drop me an email :)

@sobolevn sobolevn closed this Sep 2, 2022
@sobolevn sobolevn reopened this Sep 2, 2022
@sobolevn sobolevn merged commit 12e8009 into typeddjango:master Sep 2, 2022
@djbrown djbrown deleted the patch-1 branch September 2, 2022 15:16
djbrown added a commit to djbrown/hbscorez that referenced this pull request Mar 23, 2024
djbrown added a commit to djbrown/hbscorez that referenced this pull request Mar 24, 2024
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.

3 participants