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

Revert "Used _ContextKeys for context like dicts" #1311

Merged
merged 1 commit into from Jan 21, 2023

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Jan 5, 2023

This reverts commit cb2f233, PR #1298.

That change causes false positives with common template rendering patterns, and reasons for the change were not well explained.

See my 3 comments at #1298 (comment)

Related issues

Refs #1298, #1306.

@mschoettle
Copy link
Contributor

Sorry, this might not be the feedback you are hoping for.

As the contributor of #1306 I am in favour of reverting this. I agree that context should be dict[str, Any]. I think the Django documentation supports this as well: https://docs.djangoproject.com/en/4.1/ref/templates/api/#rendering-a-context

@intgr
Copy link
Collaborator Author

intgr commented Jan 21, 2023

Sorry, this might not be the feedback you are hoping for.

Contrary, feedback from community members and prior contributors is very much appreciated.

There's also tacit endorsement from ljodal in #1316 (comment)

For the specific PR in question here I'm also unsure what the goal of the initial PR was, but I predict a lot of errors in our codebases as well

I think I'm within my rights to consider PR this approved by community members and merge it.

Of course this reversion doesn't have to be final, if there's a good reason and a way to fix the issues with the original PR.

@intgr intgr merged commit c81f373 into typeddjango:master Jan 21, 2023
@mschoettle
Copy link
Contributor

Thank you for your work and help in advancing this project!

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.

None yet

2 participants