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.

Sign up for GitHub

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

Jump to bottom

Type hints: utils module #1707

Merged
merged 5 commits into from
Dec 9, 2022
Merged

Type hints: utils module #1707

merged 5 commits into from
Dec 9, 2022

Conversation

leandrodesouzadev
Copy link
Contributor

typing: Adds type hints to the utils module. A stubs file was added with type definitions.

Partial implementation of #1705.

tim-schilling
tim-schilling reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

It looks like get_sorted_request_variable needs a type hint as well.

debug_toolbar/utils.py Outdated Show resolved Hide resolved
debug_toolbar/utils.py Outdated Show resolved Hide resolved
tim-schilling
tim-schilling reviewed Nov 24, 2022
View reviewed changes
debug_toolbar/stubs.py Outdated
@@ -0,0 +1,24 @@
from typing import Any, List, NamedTuple, Optional, Tuple

from django import template as dj_template
Copy link
Member

Choose a reason for hiding this comment

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

@matthiask Admittedly I haven't done typing in a project of my own that requires a stubs file. I'm unsure how I feel about a single stub file without docs or some way to determine if they're used someplace. However, I think I'm okay with us using this until we find it doesn't work.

Thoughts?

tim-schilling
tim-schilling approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@tim-schilling tim-schilling 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 good with these changes, but want to see if we need a discussion regarding stubs.py before merging.

@matthiask
Copy link
Member

I'm only wondering if the module should be renamed to _stubs to make it clear that it's not supposed to be used from outside django-debug-toolbar (not even by third party panels). At the very least I'd expect a comment stating that these stubs are purely internal.

Apart from that I have to admit that I haven't used typing on real world projects yet. I have played around with mypy a bit but that's all, so I can't really say if these changes make sense or not. FWIW I approve this pull request as well :)

@tim-schilling
Copy link
Member

@leandrodesouzadev could you make the changes Matthias outlined? I think they're worthwhile.

@leandrodesouzadev
Copy link
Contributor Author

@tim-schilling I just pushed a new commit with the changes outlined by @matthiask.

@matthiask matthiask merged commit 19f1cf3 into django-commons:main Dec 9, 2022
@matthiask
Copy link
Member

Thanks!

@sshishov
Copy link

You can just check guys how the types are added into other libraries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-schilling tim-schilling tim-schilling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leandrodesouzadev @matthiask @tim-schilling @sshishov