Skip to content

Used _ContextKeys for context like dicts #1298

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

Merged
merged 12 commits into from
Dec 29, 2022
Merged

Conversation

smithdc1
Copy link
Contributor

I have made things!

I believe that ContextKeys should be allowed when using a dict "context". This updates for a few core items I came across but I've not yet done a full audit. That's a big task and there are many variations already, so thought best to get feedback first.

_ContextKeys: TypeAlias = int | str | Node

With these context like dicts, there are several different versions floating about. Even with the changes suggested here there is:

  • dict[str, Any]
  • dict[str, str]
  • Mapping[str, Any]

Further:

  • Context.flatten returns dict[_ContextKeys, dict[_ContextKeys, type[Any] | str] | int | str | None]
  • Variable.resolve accepts Mapping[str, Mapping[str, Any]]

Is it worth trying to standardise? Maybe about dict[_ContextKeys, Any]?

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.

I think dict[_ContextKeys, Any] is the way to go 👍
Please, double check Mapping case to be required.
Thanks!

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.

Other from a single comment - looks good to me!

@@ -35,6 +35,8 @@ from django.utils.functional import _StrOrPromise
from django.utils.safestring import SafeString
from typing_extensions import Literal, TypeAlias, TypedDict

from ...template.context import _ContextKeys
Copy link
Member

Choose a reason for hiding this comment

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

nit: please, use absolute import, ... is hard to read.

@@ -33,6 +33,7 @@ from django.urls.resolvers import URLPattern
from django.utils.datastructures import _ListOrTuple
from django.utils.functional import _StrOrPromise
from django.utils.safestring import SafeString
from template.context import _ContextKeys
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from template.context import _ContextKeys
from django.template.context import _ContextKeys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. 👍

Hopefully I've captured them in the required style now 🤞

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! Just a single thing left and it is good to go. Thanks!

from django.http.request import HttpRequest

TestContext = TypedDict("TestContext", {"user": Any})
TestContext = dict[Union[int, str, Node], Any]
Copy link
Member

Choose a reason for hiding this comment

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

I think that this test is here to be sure that custom TypedDict instances are allowed. Because people can type their contexts with it.

Please, do not change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the use case. However, I'm not sure on the way forward here.

As far as I can tell TypedDict only supports the use case where a dictionary object has a specific set of string keys.

The change I'm proposing here is that the key could be int | str | Node, and therefore the two are incompatible?

One option is not to change the type in the shortcut file. This would allow the test to pass, but I'm not sure it's the right solution.

Copy link
Member

Choose a reason for hiding this comment

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

I think that typed dict {'user': anything} must be compatible with dict[int | str | None, Any]. But, if it is not - I am not surprised.

I've looked into the test history - it is quite old. I don't remember the history behind it. But, it is clear that we can break this use-case for user who already use it. But, using correct type annotations is also quite important.

I think people can go with cast() when using typed dicts, so it is more important to have correct annotations 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found TypedDict not being compatible with dict somewhat surprising. However, that's been discussed previously at #355 and so maybe we need to use Mapping in place of Dict throughout?

I'm also surprised that a typed dict is not compatible with a wider type definition. The same is true of dict's too.

from typing import TypedDict, Any, Mapping

class MyContext(TypedDict):
    user: Any


def str_mapping_function(context: Mapping[str, Any]) -> None:
    pass

def multi_mapping_function(context: Mapping[str | int , Any]) -> None:
    pass

ctx = MyContext(user="MyName")

str_mapping_function(ctx) # ok
multi_mapping_function(ctx) #  error: Argument 1 to "multi_mapping_function" has incompatible type "MyContext"; expected "Mapping[Union[str, int], Any]"

Copy link
Member

Choose a reason for hiding this comment

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

Can you please try Mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to mapping, and reverted the test. Just left with one error which I'm not sure how to resolve.

main:9: error: Argument 3 to "render" has incompatible type "TestContext"; expected "Optional[Mapping[Union[int, str, Node], Any]]" (diff)

I could add a cast() like this, but this doesn't seem like a good answer to me.

test_context = cast(Mapping[Union[int, str, Node], Any], test_context)

Copy link
Member

@sobolevn sobolevn Dec 29, 2022

Choose a reason for hiding this comment

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

We can remove this test and recommend users to use cast(Any, typed_context) for their code. If they use TypedDict.

@smithdc1
Copy link
Contributor Author

Sorry for the noise. I'll have to see about installing WSL so I can get the tests to run. It's a bit hard to see the wood through the trees running the test suite on Windows.

@sobolevn sobolevn merged commit cb2f233 into typeddjango:master Dec 29, 2022
@sobolevn
Copy link
Member

No worries, thanks for the fix.

@smithdc1 smithdc1 deleted the context branch December 29, 2022 17:26
@mschoettle
Copy link
Contributor

mschoettle commented Dec 29, 2022

Just came across this for render_to_string from django.template.loader.

Out of curiosity: What is the reasoning/advantage of using Mapping[_ContextKeys, Any] over dict[_ContextKeys, Any]?

Update: Additional context: We use render_to_string to render an email template with translation strings and some context. This is how we used to call it:

context = {'foo': 'bar'}
render_to_string('path/to/email.txt', context)

mypy reports:

error: Argument 2 to "render_to_string" has incompatible type "Dict[str, str]"; expected "Optional[Mapping[Union[int, str, Node], Any]]" [arg-type]

Adding a type hint dict[Union[int, str, Node], Any] to context resolves it.

According to the documentation context should be a dict: https://docs.djangoproject.com/en/dev/topics/templates/#django.template.loader.render_to_string

@intgr
Copy link
Collaborator

intgr commented Jan 5, 2023

This is causing tons of false positives in my code base. I think this is a release blocker.

Simple code like this:

from django.template.loader import get_template

template = get_template("bla.html")
context = {"title": "test", "number": 42}
template.render(context)
# Even using `Any` doesn't work:
context2: dict[str, Any] = {"title": "test", "number": 42}
template.render(context2)

error: Argument 1 to "render" of "_EngineTemplate" has incompatible type "Dict[str, object]"; expected "Union[Context, Mapping[Union[int, str, Node], Any], None]" [arg-type]
error: Argument 1 to "render" of "_EngineTemplate" has incompatible type "Dict[str, Any]"; expected "Union[Context, Mapping[Union[int, str, Node], Any], None]" [arg-type]

@intgr
Copy link
Collaborator

intgr commented Jan 5, 2023

To be frank, I don't understand what this PR is trying to achieve.

Context dict is now assumed to be Mapping[int | str | Node, Any]

As I understand, context keys are usually names of attributes; these attribute names are then accessible in Django templates as {{ attr_name }} or {{ nested_dict.attr_name }} etc. I've never seen code using anything other than str for these keys. (Values of course can be arbitrarily complex objects, but that was already allowed due to Any).

What's the use case for using int or Node as the key in a context dict? I'm not even sure what a Node is. Is there a code example, in Django documentation or somewhere else, that demonstrates the use case?

PS: The reason that this causes issues is that Mapping keys aren't covariant (whereas values are covariant). Interesting relevant discussion here: python/typing#445 python/mypy#1114

@intgr
Copy link
Collaborator

intgr commented Jan 5, 2023

I notice that part of this PR was already reverted last week in #1306 and that has elicited no feedback here.

I'm inclined to revert the rest as well, to unblock the release, unless there is a convincing explanation for this.

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.

4 participants