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

Improve typing in template registry #1904

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jan 14, 2024

Fixes #1903

@Viicos Viicos marked this pull request as draft January 14, 2024 11:24
@Viicos Viicos marked this pull request as ready for review January 15, 2024 16:55
@Viicos Viicos changed the title WIP: Improve typing in template registry Improve typing in template registry Jan 15, 2024
Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

I'm fine with adding the tests, since e.g. the decorators aren't trivial. But I'd say we should bundle them under the same test, some "decorator test".

Mainly as the suite gets slowed down quite a bit with multiple tests.

django-stubs/template/defaultfilters.pyi Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Mar 20, 2024

I'm fine with adding the tests, since e.g. the decorators aren't trivial. But I'd say we should bundle them under the same test, some "decorator test".

Mainly as the suite gets slowed down quite a bit with multiple tests.

Will do. I'll also try to add some comments in the stubs, so that it's easier to reason about which overload matches to which use case

@intgr
Copy link
Collaborator

intgr commented Mar 25, 2024

Assigned to @flaeppe. If you don't want to lead the review here, feel free to unassign yourself.

@Viicos
Copy link
Contributor Author

Viicos commented Apr 2, 2024

I'm fine with adding the tests, since e.g. the decorators aren't trivial. But I'd say we should bundle them under the same test, some "decorator test".

Mainly as the suite gets slowed down quite a bit with multiple tests.

Thinking again about this, maybe this would things a bit confusing? We would end up with tests of the same module in a separate location.

But if still think we should do it this way, I can work on another PR moving every decorator tests, once this one gets merged (will makes things easier to review)

Comments taken from the Django source code
@flaeppe
Copy link
Member

flaeppe commented Apr 4, 2024

Thinking again about this, maybe this would things a bit confusing? We would end up with tests of the same module in a separate location.

I meant gathering the newly added tests in this PR under the same test case. e.g. test_library_decorators. That shouldn't result in multiple locations.

But if still think we should do it this way, I can work on another PR moving every decorator tests, once this one gets merged (will makes things easier to review)

If you like to refactor existing tests in a separate PR later on, that's fine too

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.

Support takes_context in template registry methods, improve typing of decorators
3 participants