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

Add type annotations #76

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

Add type annotations #76

wants to merge 6 commits into from

Conversation

jacobjove
Copy link

No description provided.

@jacobjove jacobjove mentioned this pull request Nov 25, 2023
Copy link
Owner

@craigds craigds 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 genuinely curious what the purpose of type stubs is. Sometimes my VSCode loads them for Django when I'm really just trying to look at the source code, and the type stub is an annoyance more than anything.

They seem to convey information that would be just as easily included in the code itself (and less likely to get out-of-sync). Would it be better to just add type hints to the codebase?

@jacobjove
Copy link
Author

jacobjove commented Nov 27, 2023

@craigds , I agree.

My understanding is that type stubs can be incrementally added to projects and distributed separately/independently from those projects. Looking into this, I found that it's not necessary to distribute stubs if you are willing to include type hints in the codebase: https://peps.python.org/pep-0561/#packaging-type-information

So if you're willing to include type hints in the codebase, that's preferable in my opinion.

We can add a file named py.typed to the source directory, and then that directory and its children should be analyzable by static type checkers.

Another reference: https://blog.whtsky.me/tech/2021/dont-forget-py.typed-for-your-typed-python-package/

@jacobjove jacobjove changed the title Add type stubs Add type annotations Nov 27, 2023
Copy link
Owner

@craigds craigds left a comment

Choose a reason for hiding this comment

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

This looks great. Sorry it took me so long to review it.

The generics support looks well thought out 👍

Just needs a couple of minor tweaks as mentioned 😄

Thanks for your time working on this!

self.presave(*args, **kwargs)
return super(TypedModel, self).save(*args, **kwargs)

def presave(self, *args, **kwargs) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

why a separate method for this? I'd prefer to leave this in save() or underscore-prefix it, to avoid adding to the public API

@@ -358,6 +368,8 @@ class Feline(Animal):
def say_something(self):
return "meoww"
'''
_typedmodels_type: Optional[str]
_typedmodels_subtypes: Optional[list[str]]
Copy link
Owner

Choose a reason for hiding this comment

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

list[str] requires from __future__ import annotations until Python 3.9

This project still states support for python 3.6+, although mostly because I haven't gotten around to changing that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants