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

fix: type hints weren't installed properly, add type hints for Django Fields #259

Merged
merged 3 commits into from
Sep 21, 2023
Merged

fix: type hints weren't installed properly, add type hints for Django Fields #259

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9e6007
fix: type hints weren't installed properly
bradenmacdonald Aug 24, 2023
92b9102
feat: Add smart type hints to OpaqueKeyField and its subclasses
bradenmacdonald Aug 25, 2023
7087032
fix: you can get a None value when reading an OpaqueKeyField
bradenmacdonald Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
# 2.5.1

* Fixed - py.typed file wasn't installed properly via setuptools.
* Added type hints for the OpaqueKeyField subclasses (requires mypy plugin)

# 2.5.0

* Added python type hints
Expand Down
2 changes: 1 addition & 1 deletion MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ include requirements/django.in
recursive-include opaque_keys *.html *.png *.gif *js *.css *jpg *jpeg *svg *py
include requirements/constraints.txt
include requirements/base.txt

include opaque_keys/py.typed
2 changes: 1 addition & 1 deletion opaque_keys/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from stevedore.enabled import EnabledExtensionManager
from typing_extensions import Self # For python 3.11 plus, can just use "from typing import Self"

__version__ = '2.5.0'
__version__ = '2.5.1'


class InvalidKeyError(Exception):
Expand Down
15 changes: 15 additions & 0 deletions opaque_keys/edx/django/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,12 @@ class LearningContextKeyField(OpaqueKeyField):
"""
description = "A LearningContextKey object, saved to the DB in the form of a string"
KEY_CLASS = LearningContextKey
# Declare the field types for the django-stubs mypy type hint plugin.
# See https:/typeddjango/django-stubs/blob/6a850f6/django-stubs/db/models/fields/__init__.pyi#L503-L511
# for examples of how this works for the standard field types in the upstream codebase.
# Note that these particular type annotations have no effect at runtime nor on Django itself nor on PyLance.
_pyi_private_set_type: LearningContextKey | str | None # The types that you can set into this field.
_pyi_private_get_type: LearningContextKey # The type that you get when you read from this field
Copy link
Contributor

Choose a reason for hiding this comment

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

So I've seen this pattern elsewhere, and I totally believe you're doing the right thing. But I don't understand this part–why is None a settable type, but not a gettable type for this field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ormsbee

why is None a settable type, but not a gettable type for this field?

My understanding was that for these type definitions you generally don't specify None as either a setter or a getter, and the mypy plugin will automatically add the | None union type if you declare a specific field instance with null=True. And I noticed that even if you don't declare it with null=True, you can still set a None value because the field type converts that to an empty string.

That said, I think I was not really thinking things through. OpaqueKeyFields should never be declared as null=True, because they're CharField-based, but instead they should use blank=True or not to indicate whether they accept empty values. The problem is that OpaqueKeyField will convert empty strings to None in the getter, which mypy is unaware of without null=True. So in the end, I think it's best to just explicitly specify None as one of the possible types on both the getter and the setter, which I have now done.

Thanks for the helpful question. I have pushed a commit to add | None to the getters.



class CourseKeyField(OpaqueKeyField):
Expand All @@ -202,6 +208,9 @@ class CourseKeyField(OpaqueKeyField):
"""
description = "A CourseKey object, saved to the DB in the form of a string"
KEY_CLASS = CourseKey
# Declare the field types for the django-stubs mypy type hint plugin:
_pyi_private_set_type: CourseKey | str | None
_pyi_private_get_type: CourseKey


class UsageKeyField(OpaqueKeyField):
Expand All @@ -210,6 +219,9 @@ class UsageKeyField(OpaqueKeyField):
"""
description = "A Location object, saved to the DB in the form of a string"
KEY_CLASS = UsageKey
# Declare the field types for the django-stubs mypy type hint plugin:
_pyi_private_set_type: UsageKey | str | None
_pyi_private_get_type: UsageKey


class LocationKeyField(UsageKeyField):
Expand All @@ -228,3 +240,6 @@ class BlockTypeKeyField(OpaqueKeyField):
"""
description = "A BlockTypeKey object, saved to the DB in the form of a string."
KEY_CLASS = BlockTypeKey
# Declare the field types for the django-stubs mypy type hint plugin:
_pyi_private_set_type: BlockTypeKey | str | None
_pyi_private_get_type: BlockTypeKey
Loading