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

[bradenmacdonald]

My last PR #256 wasn't quite working with mypy in edx-platform. It was working fine in "editable" mode with python setup.py develop or pip install -e . but when installed via pip install edx-opaque-keys==2.5.0 or python setup.py install, the important py.typed marker file was missing.

I thought that using include_package_data=True was sufficient, but it turns out I also needed to add a line to MANIFEST.in.

With this change, the py.typed file is installed and mypy will use the type hints added in #256.

---

I also added type hints for the django fields like UsageKeyField. This only works because of the django-stubs mypy plugin that we use in edx-platform, and requires setting a "private" class attribute to define the types, but it seems to work very well. Now mypy is aware that reading from a UsageKeyField will result in a UsageKey and writing to one accepts a UsageKey or str or None. (Keep in mind these additional static fields are type-only and will be ignored at runtime; they only affect mypy and then only if the django-stubs mypy plugin is installed.)

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (e0ee525) 94.12% compared to head (7087032) 94.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   94.12%   94.14%   +0.01%     
==========================================
  Files          29       29              
  Lines        2860     2868       +8     
  Branches      292      292              
==========================================
+ Hits         2692     2700       +8     
  Misses        142      142              
  Partials       26       26              

Flag	Coverage Δ
unittests	94.14% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
opaque_keys/__init__.py	95.94% <100.00%> (ø)
opaque_keys/edx/django/models.py	87.37% <100.00%> (+1.06%)	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mphilbrick211]

Hi @openedx/teaching-and-learning! Would someone please be able to review/merge this for us?

[bradenmacdonald]

Yes, thanks - I would appreciate a review of this one-line small change. But I can hit merge it myself once it's approved, as I've realized I need to merge openedx/edx-platform#33104 first. (Edit: that's merged now so this is good to go as soon as I can get a review)

[mariajgrimaldi]

I'm reviewing this since it was mentioned during the last CC meeting. I hope it helps!

I installed this version in a local venv:

(venv) ~/venv/lib/python3.8/site-packages/opaque_keys ls
edx  __init__.py  __pycache__  py.typed  tests

While in my current nightly installation with edx-opaque-keys 2.5.0:

app@d3bd25e0d8c4:~/venv/lib/python3.8/site-packages/opaque_keys$ ls
edx  __init__.py  __pycache__  tests

Then, I removed this comment just so type-checking warnings are raised. Then, I ran mypy with edx-opaque-keys==2.5.0 in edx-platform and got:

app@d3bd25e0d8c4:~/edx-platform$ mypy
Success: no issues found in 103 source files

After installing this version I got:

app@d3bd25e0d8c4:~/edx-platform$ mypy

openedx/core/djangoapps/content_staging/data.py:66: error: Only concrete class can be given where "Type[UsageKey]" is expected  [type-abstract]
Found 1 error in 1 file (checked 103 source files)

So now it's working! Thanks. Let me know if I miss something :)

[ormsbee]

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?

[bradenmacdonald]

@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.

[antoviaque]

@bradenmacdonald FYI your call for review has been discussed during the contributors meetup this week - there are potentially 3 reviewers: @mariajgrimaldi just had a look, Stefania will ask @ghassanmas and @auraz will try to get someone from Raccoon Gang.

[bradenmacdonald]

Thanks for testing this @mariajgrimaldi!

I still technically need an approval from someone with merge rights on this specific repo before I can merge, but it should be easier to find with your test done.

[antoviaque]

@itsjeyd See @bradenmacdonald 's request above ^ Do we have a list of people who can give a 👍 on this repo? I see from https://openedx.atlassian.net/wiki/spaces/COMM/pages/3632070677/Pilot+Phase+3 that it's being taken on by Axim for maintainership in phase 3, though that's likely not the only ones who can give a binding 👍 ?

And @bradenmacdonald any suggestion who would be a good person for the binding review, even if that person doesn't have access to it yet?

[bradenmacdonald]

@felipemontoya would you be able and comfortable approving this PR like you did the last one? :)

Do we have a list of people who can give a 👍 on this repo?

@antoviaque Isn't it as simple as this list? There are three people, but one is me. I probably should have just pinged them directly as I did last time since that eventually worked.

[openedx-webhooks]

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[bradenmacdonald]

@felipemontoya @antoviaque Never mind - as you can see now that @ormsbee is back I got his help :) Thx anyways!

[itsjeyd]

@antoviaque @bradenmacdonald Glad to see that you already found a reviewer :)

[auraz]

Hey @antoviaque JFYI I'm not in Raccoon Gang for 4+ years.